Dev15 is producing unverifiable IL

[jaredpar]

Repo steps:

1. Clone https://github.com/jaredpar/VsVim
2. Open in Dev15 Preview 5 or above. It will likely ask to upgrade the solution. Allow it to do so, it won't affect the repo
3. Build
4. Run peverify on Src\VimCore\bin\Debug\Vim.Core.dll

The tool will emit the following verification errors:

[IL]: Error: [c:\Users\jaredpar\code\VsVim\Src\VimCore\bin\Debug\Vim.Core.dll : <StartupCode$Vim-Core>.$StatusUtil+GetStatusUtilForBuffer@50::Invoke][offset 0x00000000] Unrecognized argument number.
[IL]: Error: [c:\Users\jaredpar\code\VsVim\Src\VimCore\bin\Debug\Vim.Core.dll : <StartupCode$Vim-Core>.$StatusUtil+GetStatusUtilForView@54::Invoke][offset 0x00000000] Unrecognized argument number.
[IL]: Error: [c:\Users\jaredpar\code\VsVim\Src\VimCore\bin\Debug\Vim.Core.dll : <StartupCode$Vim-Core>.$FoldManager+GetFoldData@243::Invoke][offset 0x00000000] Unrecognized argument number.
[IL]: Error: [c:\Users\jaredpar\code\VsVim\Src\VimCore\bin\Debug\Vim.Core.dll : <StartupCode$Vim-Core>.$FoldManager+GetFoldManager@246::Invoke][offset 0x00000000] Unrecognized argument number.
4 Error(s) Verifying Vim.Core.dll

That corresponds to the following F# code

    /// Get or create an PropagatingStatusUtil instance for the ITextBuffer
    member x.GetStatusUtilForBuffer (textBuffer : ITextBuffer) =
        textBuffer.Properties.GetOrCreateSingletonProperty(_key, (fun _ -> PropagatingStatusUtil()))

    /// Get or create an StatusUtil instance for the ITextView
    member x.GetStatusUtilForView (textView : ITextView) =
        textView.Properties.GetOrCreateSingletonProperty(_key, (fun _ -> StatusUtil()))

    member x.GetFoldData (textBuffer : ITextBuffer) = 
        textBuffer.Properties.GetOrCreateSingletonProperty(_dataKey, (fun _ -> FoldData(textBuffer)))

It looks like that particular pattern is causing issues with the compiler.

[jaredpar]

It looks like the bug is specific to using ignored parameters (the _). Giving the parameters explicit names fixes the issue.

https://github.com/jaredpar/VsVim/commit/672554dbb206d342bc40263bd2fb0eb62803b112

[forki]

urgs. that looks critical.

[jaredpar]

@forki looking at my code a bit more I'd venture to guess this bug is limited to:

• Use of a lambda with an ignored parameter targeting a non-F# delegate type.

I have ~60 uses of fun _ in my code and only 4 seem to hit this bug. The common link between them is targeting a C# API.

[jaredpar]

Note if you want to avoid the upgrade wizard sync to https://github.com/jaredpar/VsVim/commit/1b1145169bd092132257711a16b19d6c87321908

[KevinRansom]

@jared, @dsyme

I have taken a pretty good look at this, and I am sort of confused. First .... I'm not sure that your code is correct: GetOrCreateSingletonProperty is defined here:

• https://msdn.microsoft.com/en-us/library/ee155723(v=vs.100).aspx

It's second argument is a Func --- I think the correct f# mapping to Func is

(fun () -> Do something returning TResult)

so you probably need to fix your code to look like:

        textBuffer.Properties.GetOrCreateSingletonProperty(_dataKey, (fun () -> FoldData(textBuffer

I think

(fun _ -> Do Something returning TResult)

Has a single ignored argument passed by caller that is ignored by the func.

What bothers me is that the compiler doesn't complain that you have passed a func that has an argument albeit ignored as a paramater that expects a func without an argument. I think that may be a bug, @dsyme will have to confirm my analysis.

Other than that I think the compiler may be doing the right thing here, on the other hand the compiler doesn't really need to load the argument and then discard it in the body of the func either.

Kevin

[forki]

@kevinransom are you saying it's not OK to write

fun _ ->... 

instead of

fun () ->... 

We do that in thousands of FAKE build scripts. That would be really breaking if that's no longer the case.

[isaacabraham]

Same here, and not just in fake scripts.

[forki]

so I can't ignore unit params anymore?

[Krzysztof-Cieslak]

IMO, it's critical, breaking bug.

[forki]

trying to reproduce in #1786

[matthid]

fun _ ->...

instead of

fun () ->...

That would really be a huge breaking change and completely inconsistent with the rest of the language:

match () with
| _ -> printfn "test"

prints "test" obviously.

The spec says:

wildcard pattern

The underscore character _, which matches any input.

The feeling of the language is that unit is a valid type with a valid value (). We cannot break this. The language already fails to abstract this properly on some occasions like inheritance, we cannot open another hole here.

[forki]

can you guys please take a look at #1786 - these cases work for me with F# 4.0. Did I miss something?

[KevinRansom]

@forki -- what I am saying is that when calling a C# Function that requires an argument of Type Func then you should use (fun () -> Do something returning a TResult)

The compiler appears to allow you to pass

1. (fun x -> Do something returning a TResult) and
2. (fun _ -> Do something returning a TResult)
3. Does not cause a peverify error unless your code uses the argument.
4. Causes a peverify error because the compiler appears to generate code to discard the argument.

Normal uses of the f# idiom (fun _ -> do something interesting) are of course fine and proper. This is just a C# interop thing and not anything else.

I hope this helps clear things up.

[forki]

No tbh it doesn't. As vsvim showed this worked before and I don't understand why this broke.

Am 20.11.2016 7:42 nachm. schrieb "Kevin Ransom (msft)" < notifications@github.com>:

@forki https://github.com/forki -- what I am saying is that when calling a C# Function that requires an argument of Type Func then you should use (fun () -> Do something returning a TResult)

The compiler appears to allow you to pass

1. (fun x -> Do something returning a TResult) and
2. (fun _ -> Do something returning a TResult)
3. Does not cause a peverify error unless your code uses the argument.
4. Causes a peverify error because the compiler appears to generate code to discard the argument.

Normal uses of the f# idiom (fun _ -> do something interesting) are of course fine and proper. This is just a C# interop thing and not anything else.

I hope this helps clear things up.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/Microsoft/visualfsharp/issues/1749#issuecomment-261796270, or mute the thread https://github.com/notifications/unsubscribe-auth/AADgNGx7ZMzeSCkzTgbgrP124_mKrM27ks5rAJSNgaJpZM4KzeX2 .

[KevinRansom]

okay here is a simple repro to help you understand the issue better:

using System;

namespace ClassLibrary1
{
    public class Class1
    {
        public static void GetIt(Func<object> func)
        {
            Console.WriteLine("{0}", func());
        }
    }
}

When called from F# using:

[<EntryPoint>]
let main argv = 
    printfn "%A" (ClassLibrary1.Class1.GetIt(fun _ -> "" :> obj))
    0 // return an integer exit code

peverify yields:
[IL]: Error: [Z:\Documents\Visual Studio 2017\Projects\ConsoleApplication22\ConsoleApplication22\bin\Debug\ConsoleApplication22.exe : Program+main@5::Invoke][offset 0x00000000] Unrecognized argument number.
1 Error(s) Verifying ConsoleApplication22.exe

executing the exe yields:
Z:\Documents\Visual Studio 2017\Projects\ConsoleApplication22\ConsoleApplication22\bin\Debug>ConsoleApplication22.exe

Unhandled Exception: System.InvalidProgramException: Common Language Runtime detected an invalid program.
   at Program.main@5.Invoke()
   at ClassLibrary1.Class1.GetIt(Func`1 func) in \\mac\home\documents\visual studio 2017\Projects\ConsoleApplication22\ClassLibrary1\Class1.cs:line 9
   at Program.main(String[] argv) in \\mac\home\documents\visual studio 2017\Projects\ConsoleApplication22\ConsoleApplication22\Program.fs:line 5

When called from F# using:

    printfn "%A" (ClassLibrary1.Class1.GetIt(fun () -> "" :> obj))

peverify yields:
All Classes and Methods in ConsoleApplication22.exe Verified.

Execution yields:
<null>

[forki]

That's just bad. We should not generate invalid IL. Never. Let the compiler fail if needed (which don't think we should do here)

Am 20.11.2016 8:10 nachm. schrieb "Kevin Ransom (msft)" < notifications@github.com>:

okay here is a simple repro to help you understand the issue better:

using System;

namespace ClassLibrary1 { public class Class1 { public static void GetIt(Func

[KevinRansom]

I believe the bug here is that the compiler should offer an error when trying to pass an F# Func<T,TResult> to an API expecting a Func<TResult>,

[KevinRansom]

And I'm pretty sure this is not a new bug.

[forki]

Yes compiler error is definitely 1000x better than invalid IL. But it's still breaking and I personally don't think that's good.

Am 20.11.2016 8:12 nachm. schrieb "Kevin Ransom (msft)" < notifications@github.com>:

I believe the bug here is that the compiler should offer an error when trying to pass an F# Func to an API expecting a Func,

— You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub https://github.com/Microsoft/visualfsharp/issues/1749#issuecomment-261798193, or mute the thread https://github.com/notifications/unsubscribe-auth/AADgNGQq3wuDoVZ3R1xzz6Fdop0OT7Ouks5rAJutgaJpZM4KzeX2 .

[vladima]

I also don't think this is bug - this behavior was one of few type driven conversions supported by F# compiler and signalling it as error all the time will be a breaking change. I'll try to look into this issue in the next couple of days

[forki]

Unverified IL is not a bug? And according @jaredpar it worked before - so something changed.

(on the plus side: my WIP pr suggests that the most common cases still work. So we're talking about a much smaller case than I personally thought)

[KevinRansom]

It appears that something did change.
The F# 4.0 compiler and presumably previous ones did some magic ... and treats (fun x -> ... ) as (fun x:unit -> ) when passed to a Func<TResult>. Now it's excavation time to try to figure out what and when ... groan ...

[forki]

Yeah together with the report from @vasily-kirichenko which we still didn't resolve I'm not really confident for the next release. I tried to repro but it's still open.

F# 4.1 will probably be a really bumpy release. ;-(

[vladima]

@forki generation of non-verifyable code is definitely a bug, however a situation when compiler converts syntactic function to a delegate type is perfectly normal

[vladima]

at glance the issue is that fun _ -> ... is converted into the match with a wildcard case which is not removed when optimizations are disabled. I think that stripping of this single match when producing typed AST should do the trick - with this change both simplified repro and original issue are compiled fine and pass peverify.

// cc @dsyme to confirm if this is a viable approach.

[forki]

@vladima cool. Do you know why this worked before and why / where it broke?

[dsyme]

@vladima @forki I'll take a look today, it's just a bug, no doubt due to a change by me. We should accept the code and generate correct IL.

[dsyme]

@KevinRansom Just to clarify that in all these cases:

(fun () -> Do something returning a TResult) 
(fun x -> Do something returning a TResult) 
(fun _ -> Do something returning a TResult)

the pattern (whether (), x or _) is inferred to have type unit. There is logic in the compiler to impedance match between the empty argument list and the value of type unit. Something must be going awry with that logic, I'll look into it now.

[vladima]

@dsyme I think in the first two cases function body will be the actual function whereis in the second case it will be a match with a single wildcard match case. This match can be optimized away so problem does not repro when optimizations are enabled. For all other cases we can probably just treat them similarly.

[dsyme]

Proposed fix is here: https://github.com/Microsoft/visualfsharp/pull/1795/

[dsyme]

The culprit was the implementation of byref returns, which added the max 1 call here https://github.com/Microsoft/visualfsharp/commit/6d618a6d96f7e4cff3c89d4ee54fa88c665cf0c4#diff-456bef6dcdc9277074038269046cab70R773

That call is, logically speaking, correct - if there are no logical arguments in the byref type then we strip the lambda as if it had one argument. I just suspect all test cases had uses (fun () -> ...) rather than (fun x -> ...).

Anyway it will be interesting to see if the proposed fix has any issues/interactions with the byref-return feature. I don't think it will.

[dsyme]

Some more explanation

• Consider Delegate(fun args -> expr) where the Delegate Invoke takes no arguments
• In F# fun () -> e is a function taking one unit-valued argument. In .NET-codegen the delegate has no arguments.
• In F# the body of the function might refer to the argument e.g. Delegate(fun (x:unit) -> let y = x in e). Indeed it does in debug mode code because we bind arguments to local names during code gen so people can see them in the debugger.
• In F# the body of the function might refer to the argument e.g. fun (x:unit) -> let y = x in e and does in debug mode code because we like to bind arguments to names so people can see them in the debugger.
• So Delegate (fun (x:unit) -> e) has to become new Delegate( empty-args -> let x = () in e ). We were just forgetting the let x = (), which only matters if x is "used".

[dsyme]

Fixed by https://github.com/Microsoft/visualfsharp/pull/1795