Fsharp: Bug: --standalone fails on generic method on generic interface on generic type

[manofstick]

updated with preliminary investigation

Issue found when building #1570

Due to a generic method from a generic method on a generic class. The problem stems from ilread.fs line 2239 (I'll provide a link when I'm on something other than a mobile phone!). It calls readBlobHeapAsMethodSig passing 0 as the numtypes parameter. This value should, I think, be 2 to correspond to the following parameter count in the ISeqFactory floating types (in my case as listed below from which this issue was noticed). I had a little looks to see if I could understand how to traverse the data structures, buy they appear fairly non-trivial with lots of plain indexes being handed around.

Anyway, the code carries on for a bit before eventually crashing in pass3 of the binary writer when it tried to find the proper method, but due to this issue with ilread the matching signature cannot be found.

Running fsc with the arguments listed from ci_part2 below allow for relatively easy debugging to this point.

Anyway, back to the original text:

+++ Libraries\Portable (parse_tests.fs (Desktop)) +++
Compiling: [fsc   --standalone -g -a parse_tests.fs ]
Microsoft (R) F# Compiler version 4.1
Copyright (c) Microsoft Corporation. All Rights Reserved.
The local method 'Microsoft.FSharp.Collections.SeqModule.Composer.SeqComponentFactory`2'::'Microsoft-FSharp-Collections-SeqModule-Composer-Internal-ISeqFactory`2-Create' was referenced but not declared
generic arity: 1
A method in 'Microsoft.FSharp.Collections.SeqModule.Composer.SeqComponentFactory`2' had the right name but the wrong signature:
generic arity: 1
mdkey2: Microsoft.FSharp.Compiler.AbstractIL.ILBinaryWriter+MethodDefKey

error FS2014: A problem occurred writing the binary 'C:\src\visualfsharp\tests\fsharpqa\Source\Libraries\Portable\parse_tests.dll': Error in pass3 for type Microsoft.FSharp.Collections.SeqModule, error: Error in pass3 for type Composer, error: Error in pass3 for type SeqComponentFactory`2, error: Error in GetMethodRefAsMethodDefIdx for mref = ("Microsoft-FSharp-Collections-SeqModule-Composer-Internal-ISeqFactory`2-Create",
 "SeqComponentFactory`2"), error: Exception of type 'Microsoft.FSharp.Compiler.AbstractIL.ILBinaryWriter+MethodDefNotFound' was thrown.
Compile Unexpectedly Failed: 256

It causes failures in three places:

+++ Libraries\Portable (parse_tests.fs (Desktop)) +++
+++ Libraries\Portable (parse_tests.fs (Portable)) +++
+++ CompilerOptions\fsc\standalone (W_MissingTransitiveRef.fs) +++

This occurred after I had added the publicly accessible interface (which is I guess a bit unusual in that it is a generic interface that contains a generic method)

type ISeqFactory<'T,'U> =
    abstract PipeIdx : PipeIdx
    abstract Create<'V> : IOutOfBand -> ``PipeIdx?`` -> Consumer<'U,'V> -> Consumer<'T,'V>

Related types are:

type PipeIdx      = int
type ``PipeIdx?`` = Nullable<PipeIdx>

type ICompletionChaining =
    abstract OnComplete : PipeIdx -> unit
    abstract OnDispose : unit -> unit

type IOutOfBand =
    abstract StopFurtherProcessing : PipeIdx -> unit

[<AbstractClass>]
type Consumer<'T,'U> () =
    abstract ProcessNext : input:'T -> bool

    abstract OnComplete : PipeIdx -> unit
    abstract OnDispose : unit -> unit

    default __.OnComplete _ = ()
    default __.OnDispose () = ()

    interface ICompletionChaining with
        member this.OnComplete terminatingIdx =
            this.OnComplete terminatingIdx

        member this.OnDispose () = 
            try this.OnDispose ()
            finally ()

Repro steps

Build ci_part1 for #1570 (I haven't explorered if there are command line options to just run the single test?). There are other issues with that build, but I believe they are orthogonal.

Update: ci_part2 runs the command:

C:\src\visualfsharp\tests\fsharp\tools\bundle> C:\src\visualfsharp\release\net40\bin\fsc.exe  -r:System.Core.dll --nowarn:20 --define:COMPILED --progress --standalone -o:test-one-fsharp-module.exe -g test-one-fsharp-module.fs

which also demonstrates this issue.

Expected behavior

For the compiler not to unexpectedly fail

Actual behavior

The compiler unexpectedly fails

Known workarounds

None

Related information

Occurs on local build as well as AppVeyor, so I assume it is consistent.

[manofstick]

@dsyme

I think I really need your help here (or a week of time if you can give me that!)

Ultimately, from what I can gather, the type that ends up in ILMethodImplDef.OverrideBy is screwed in the case of a generic method on a generic interface. (which I assume is rare enough that it just hasn't occured as an erroneous test case before - this only appears in my case with the unit tests as --standalone is trying to do the binary writing by slurping this method over...)

Effectively we have Interface<!0,!1>.method<!2> that, due to the interface not being taken into account being mapped to Interface<!0,!1>.method<!0> and thus all the resultant object then not matching (obviously) because of the incorrect mapping.

To me there appear to possibly be a few places in the function seekReadMethodDefAsMethodDataUncached where this is potentially incorrect, but I'm not familiar with IL representation _at all_ in order to make a good valid approach to solving this issue.

The things I see as potentially bad are the 0 parameter passed to readBlobHeapAsMethodSig which then maps the !0 to the method, but getting a "good" value for this in my mind would need to get the _potentially_ floating parameters from the interface (as the deriving class could have specialized them), but I don't know how to get the interface that is associated with the type.

The other things I see as potentially bad is the generation of minst which _does_ take the finst.Length as a parameter to seekReadGenericParams, but then this is ignored (well it doesn't exist by the time it reaches it) in the creation of of the floating type in mkILFormalGenericArgs.

Anyway, I'm not even 100% happy with the "Formal" terminology.

So, as I said, give me a week, and I can read the spec and then nut this out, otherwise maybe you can drag up some ancient memory, wave your magic wand, and wish this bug away...

[manofstick]

...rant...

I would really like some feedback to these kind of things, even if it's a "sorry, we're really under the pump at the moment - keep up the good work!". My time is approaching my burn out phase - i.e. I want to get #1570 over the line, but this requires pretty much every evening and some time over the weekends to get it done. My family would obviously prefer me to _not_ be spending my non-work time in front of a computer. Anyway, all I'm saying is - I'm running a bit of a marathon and even if your not running with me, and little cheer from the side lines would be appreciated. I have absolutely no idea if you actually _want_ #1570 to be finished??? Being a volunteer requires a bit of stupidly on my behalf (like what the hell am I getting out of this - not very economic rationalist of me...) and the answer is I get _energised_ by people liking/using/debating this stuff... There are obviously a few people who are helping me, and a few people on the side lines giving the thumbs up, but the people - the company even - that I'm actually donating my time to the most have all been silent when I have asked for assistance. Maybe you guys are running on such a skeleton crew that you can't find the time to throw a bit of banter - even if it is "sorry, the guy who did that has left now, so you're pretty much on your own". I find this disappointing. The main reason why I never got around to patching the equality/comparer change was that too much time passed between me getting in the zone, and then the final feedback that I got. By that stage - months - my intimacy with the code was lost. You might as well of just modified the code yourself (why didn't you!??!?!). Anyway, if you are truly the skeleton crew, then you should be fostering relationships ...

/rant...

Anyway... I believe I have fixed this in #1704

[7sharp9]

@manofstick I feel for you, monumental effort is required for these sorts of things.

I felt the same on #882 I didn't get any feedback that really helped progress the PR, and while I worked on that while working at Xamarin I put most of the hours in outside of work.

PR's are the life blood of OSS too.

[manofstick]

@7sharp9

I do find it kind of funny. You would think that leveraging skills of the world might be beneficial, but I think the reality is that's it just annoying. i.e. your pay is only based on what you create probably, not "wasting" time communicating with some idiot on the internet! :-)

Anyway, that's for responding to my rant!

Cheers!

[manofstick]

@7sharp9 (when I said "your" I mean in from the point of view of one of the microsoft's f# employees, hopefully that was clear! But on second reading I thought it would be misconstrued)

[forki]

@manofstick currently the whole team seems to be under big pressure to get roslyn support and netcore stuff ready for VS Vnext. Unfortunately this has drawn all resources since last release of F#. Personally I hope that things improve after the VS release and the team allocates more dedicated time to look at reported issues and PRs.

Please don't give up. Believe me there are many people reading this and cheering silently.

[7sharp9]

@manofstick At first I thought that you thought I worked for MS, which I don't, then on second read I know what you mean. :-)

[dsyme]

@manofstick Thanks for finding and fixing this issue, it's great to have it fixed.

I gave some feedback on the Seq composer PR here. My apologies for tuning out of that, it was for the reasons I explained.

I will admit my current major priority is to reduce the complexity of engineering in this repository. I'm doing this mainly to help enable a simpler overall development story for F# on both .NET Framework and .NET Core. Some of my priorities are

• [x] Making sure you can compile the compiler for .NET Framework without picking up any CoreCLR dependencies. (done, just use build net40).
• [ ] Get the repo compiling the F# compiler for .NET Framework on Linux/Mono so we can catch issues earlier (in progress, #1703)
• [ ] Remove "stupid" tool dependencies in the code, such as on "old" LKGs, or needless use of compiled MSBuild task DLLs, or needless use of basically trivial compiled tools like FSsrgen.exe and subst.exe (in progress, #1703 and other commits removing 100s of lines from FsLexYacc)
• [ ] Work towards a Linux/.NET Core-only build of the F# compiler that doesn't even need Mono installed. This is a stretch goal that's a fair way off

So I'm currently happier deleting things than adding them.

[manofstick]

@dsyme

Rereading this whole thread I see some of the ambiguities of the English language probably didn't help me. My use of "give me a week" was meant in the literal gifting sense, not in the sense of I'll be fine given enough time. Ok, so it turned out to be easier to grok than I had been thinking it might be, so all good, but anyway...

... And from looking at your activities over the weekend your family will probably be wanting more of your time as well!