Fix ProjectFileNames order when getting project options from script

[alfonsogarciacaro]

This is more of an issue/question than a PR but I wanted to include the code to better illustrate my point.

I would like to make it possible just to use F# scripts instead of a fsproj file. However, compilation of scripts quickly fails when the file structure complicates a bit. Consider we have the following four files:

• BaseLib.fs
• Lib1.fsx (loads BaseLib.fs)
• Lib2.fsx (loads BaseLib.fs)
• Main.fsx (loads Lib1.fsx, Lib2.fsx)

When I call FSharpChecker.GetProjectOptionsFromScript on Main.fsx I get the following file ordering, which makes compilation fail:

Lib1.fsx
BaseLib.fs
Lib2.fsx
Main.fsx

Of course this is fixed if I load BaseLib.fs before anything in Main.fsx but, given that GetProjectOptionsFromScript actually finds BaseLib.fs, I wonder if it would be possible to fix the ordering so Main.fsx doesn't need to know Lib1.fsx dependencies in advance.

The fix in this PR does actually this, however I got it by trial-and-error and I didn't test with other file structures so probably there's a much better solution. My question, would something along these lines be possible (so it's worth working on it) or is there any reason it's bound to fail in certain situations?

Thanks!

[cloudRoutine]

Shouldn't this be addressed inside FSharp.Compiler.Service?

I think the AgedLookup inside the MruCache is removing the duplicates here
https://github.com/fsharp/FSharp.Compiler.Service/blob/0cae295c1f9be2caafe152d7869ab944086c46d3/src/fsharp/InternalCollections.fs#L72-L86

[7sharp9]

I thought this already happened? How would scripts ever have worked?

[alfonsogarciacaro]

Seems I didn't make a good job to explain the problem. I'll try to fix that:

• Duplicates are not a problem and are indeed removed, that's why BaseLib.fs is not loaded twice even if it's loaded by both Lib1.fsx and Lib2.fsx.
• From my understanding (I might be wrong), this is how FCS is reading the source files at the moment when getting the project options from the script:

1. FCS reads the first #load directive of Main.fsx so it adds Lib1.fsx to the list of source files.
2. FCS then reads the #load directives of Lib1.fsx (in this case BaseLib.fs) and puts them after the previously loaded files.
3. FCS reads the next #load in Main.fsx, which is Lib2.fsx and repeats step 2. But as BaseLib.fs has already been added it doesn't add anything to source files besides Lib2.fsx.

In super-simplified and imperative (because that is what we love 😉) code this would become:

    type LoadDirective = { file: File }
    and File = { path: string; loadDirectives: LoadDirective list }

    let rec getProjectSourceFiles (sourceFiles: ResizeArray<string>) (file: File) =
        for loadDirective in file.loadDirectives do
            sourceFiles.Add(loadDirective.file.path)
            getProjectSourceFiles sourceFiles loadDirective.file

    let sourceFiles = ResizeArray<string>()
    getProjectSourceFiles sourceFiles mainFile

So my naive thinking it's the problem can be solved by swapping the lines within the for loop in the above code (so the dependencies of a file are added to the sourceFiles list before the file itself). Is that correct?

I hope it's a bit clearer now and I didn't mess things further. If you want me to create a repo with the sample files I'm talking about, just tell me.

[matthid]

This is the fix for #587, correct?

[matthid]

It clearly looks like someone was doing "distinct" on the reverted list.

[alfonsogarciacaro]

Yeah, it seems to be related to #587, I didn't notice that. Thanks for pointing it out!

[dsyme]

@alfonsogarciacaro Do you know if this bug manifests in the Visual F# Tools (fsi.exe and its language service)? Should we be making a corresponding fix there?

[0x53A]

@dsyme
Here (fsprojects/FAKE#1263) is a comparison between FSI and FSharp.Compiler.Service:

1.fsx

#load "2.fsx"
#load "3.fsx"

printfn "HELLO FROM 1"

2.fsx

#load "4.fsx"

printfn "HELLO FROM 2"

3.fsx

#load "4.fsx"

printfn "HELLO FROM 3"

4.fsx

printfn "HELLO FROM 4"

C:\Users\lr\Source\Repos\Test>"C:\Program Files (x86)\Microsoft SDKs\F#\3.1\Framework\v4.0\fsi.exe" 1.fsx
HELLO FROM 4
HELLO FROM 2
HELLO FROM 4
HELLO FROM 3
HELLO FROM 1

C:\Users\lr\Source\Repos\Test>packages\fake\tools\fake.exe 1.fsx
HELLO FROM 2
HELLO FROM 4
HELLO FROM 3
HELLO FROM 1

(I run fake.exe, but it uses FSharp.Compiler.Service internally)

FSI executes script 4 twice, while FSharp.Compiler.Service executes each script exactly once, but in the wrong order.

[alfonsogarciacaro]

I'm on OSX/Mono and I'm getting similar results, fsharpi loads files in the proper while fsharpc fails. I'll try to investigate what's the difference between both workflows.

MyLib.fs

let add2 x = x + 2

MyLib1.fsx

#load "MyLib.fs"
let add3 = MyLib.add2 >> ((+) 1)

MyLib2.fsx

#load "MyLib.fs"
let add4 = MyLib.add2 >> ((+) 2)

Main.fsx

#load "MyLib1.fsx"
#load "MyLib2.fsx"
MyLib1.add3 5 |> printfn "%i"
MyLib2.add4 5 |> printfn "%i"

$ fsharpi Main.fsx 
8
9

$ fsharpc Main.fsx
F# Compiler for F# 4.0 (Open Source Edition)
Freely distributed under the Apache 2.0 Open Source License

/temp/MyLib1.fsx(4,6): error FS0039: The namespace or module 'MyLib' is not defined

/temp/MyLib1.fsx(7,12): error FS0039: The value or constructor 'add2' is not defined

[alfonsogarciacaro]

There are two similar options here.

The first one is called by GetProjectOptionsFromScript, which Fable uses and what I tested for this PR. According to the comment, the second one is called both by fsi and fsc but they deal with the result differently:

fsi
https://github.com/fsharp/FSharp.Compiler.Service/blob/master/src/fsharp/fsi/fsi.fs#L1217

fsc
https://github.com/fsharp/FSharp.Compiler.Service/blob/master/src/fsharp/fsc.fs#L233

[alfonsogarciacaro]

As @matthid pointed out, the problem seems to be with grouping in this line: List.groupByFirst.

I undid changes in the previous commit and just used closure's Inputs instead of SourceFiles (the same way as fsi does) to build the Project Options here and the fix still applies. At least for Fable which uses GetProjectOptionsFromScript, I guess this is not yet enough to fix fsc.

[alfonsogarciacaro]

I've changed the name of the PR. Please let me know if it can be accepted just as a patch for GetProjectOptionsFromScript or you want me to give it a shot at fixing fsc too. I'm not sure yet how to do that though. Maybe just removing Lists.groupByFirst from this line as commented above?

[dsyme]

@alfonsogarciacaro Could you add a test case please? That would clarify the impact of the change for me. Thanks

[dsyme]

@alfonsogarciacaro We've now integrated all the changes from visualfsharp. Could you merge with master and update the PR please? Thanks

[dsyme]

Closing and reopening to trigger CI

[alfonsogarciacaro]

Hi Don, I've rebased with latest master and added the test. Checking the test I realised that my second "fix" was actually wrong (replacing SourceFiles with Inputs is not enough) so I went back to the original solution.

In any case, I'd appreciate if you can have a look. This is working now for my use case but I haven't managed to run the other tests, so I don't know if I'm breaking something else. build.fsx refuses to work on my machine and Travis is hanging forever 😕

[alfonsogarciacaro]

@dsyme, with the latest commit, only the tests that are also failing on master are failing now. Again, it'd be best if someone with a better knowledge of the code could review it, as I did the changes mostly by trial-and-error. For example, I'm not sure if I need to do something else with the other members of LoadClosure.

[alfonsogarciacaro]

I realized my previous fix wasn't working in all situations so I've added more tests and improved it. What confused me was the GetAvailableLoadedSources function, which loaded more files than expected (I thought it was only loading dependencies of the current file). I've fixed that by setting a file as observed right after confirming it hasn't been. This seems to work and the new tests are passing but, as usual, I'm exploring uncharted territory so reviews are appreciated.

[alfonsogarciacaro]

@dsyme, I rebased the PR against the latest changes in master and now build checks are passing 🎉 Also, I published a Fable version with a custom build including this fix and there have not been errors reported so far. If the PR is merged, how should this fix be propagated to the F# compiler? Should I send another PR to the fsharp or visualfsharp repo? Cheers!

[dsyme]

I've confirmed that the broken behaviour reported above also repros on F# 4.0 on Windows.

It also repros when fsi --load:Main.fsx is used.

So yes, we wiwll also need to propagate this fix to the visualfsharp repository. I'll just cherry-pick it and send a PR there since it's quite important to fix

[dsyme]

Fix in visualfsharp is here: dotnet/fsharp#1363

[dsyme]

@alfonsogarciacaro We have failing tests in visualfsharp after this change, see dotnet/fsharp#1363

We'd better get those sorted out

[alfonsogarciacaro]

@dsyme, thanks for that! I'll try to move the tests to FCS and fix them here 👍

[dsyme]

@alfonsogarciacaro I think the errors are all of this form below. This is for tests\fsharp\core\attributes, running any of these:

..\..\..\..\Debug\net40\bin\FsiAnyCPU.exe  test.fsx
..\..\..\..\Debug\net40\bin\FsiAnyCPU.exe  testlib.fs test.fsx
..\..\..\..\Debug\net40\bin\FsiAnyCPU.exe  testlib.fsi testlib.fs test.fsx

from

C:\GitHub\dsyme\visualfsharp\tests\fsharp\core\attributes

We get the same error whether testlib.fsi and testlib.fs occur on the command line or not. Note the line

#load "testlib.fsi" "testlib.fs" 

in test.fsx. Without the testlib.fsi on the #load line it works correctly. The fsi-reload error looks like the same root cause.

C:\GitHub\dsyme\visualfsharp\tests\fsharp\core\attributes>..\..\..\..\Debug\net4
0\bin\FsiAnyCPU.exe -r:System.Core.dll --nowarn:20 --define:INTERACTIVE --maxerr
ors:1 --abortonerror testlib.fsi testlib.fs test.fsx


warning FS0075: The command-line option '--abortonerror' is for test purposes on
ly


testlib.fsi(4,1): error FS0238: An implementation of file or module 'FSI_0001_Te
stLibModule' has already been given. Compilation order is significant in F# beca
use of type inference. You may need to adjust the order of your files to place t
he signature file before the implementation. In Visual Studio files are type-che
cked in the order they appear in the project file, which can be edited manually
or adjusted using the solution explorer.

[alfonsogarciacaro]

That's very valuable info! We may fix it by passing and initial value to the observed sources (the sources that were passed on the command line). I'll give it a try.

[dsyme]

@alfonsogarciacaro Updated the information above - the bug appears with

#load "testlib.fsi" "testlib.fs" 

in the script

[dsyme]

@alfonsogarciacaro The bug in tests\fsharp\core\reflect is different

..\..\..\..\Debug\net40\bin\FsiAnyCPU.exe test.fs test2.fs

The order test2.fs and test.fs is reversed from the command line arguments:

[Loading C:\GitHub\dsyme\visualfsharp\tests\fsharp\core\reflect\test2.fs
 Loading C:\GitHub\dsyme\visualfsharp\tests\fsharp\core\reflect\test.fs]

test2.fs(25,6): error FS0039: The namespace or module 'Test' is not defined

[dsyme]

@alfonsogarciacaro Finally, the LoadingFsx.fsx and LoadingFsx.fsscript failures are because it seems in F# 4.0 a script can #load itself without failure, e.g. consider

C:\GitHub\dsyme\visualfsharp\tests\fsharpqa\Source\InteractiveSession\Misc\LoadingFsx.fsx

which contains

// #NoMT #FSI 
// It is ok to #load an .fsx file
//<Expects status="success"></Expects>

let x1 = 1

#if INTERACTIVE

#load "LoadingFsx.fsx"

#endif

let x2 = 2

#q;;

The test is running

..\..\..\..\Debug\net40\bin\fsc.exe LoadingFsx.fsx

from

C:\GitHub\dsyme\visualfsharp\tests\fsharpqa\Source\InteractiveSession\Misc

[dsyme]

Removing the List.rev from this line looks promising:

        let protoClosure = files |> List.map (fun (filename,m)->SourceFileOfFilename(filename,m,tcConfig.inputCodePage)) |> List.concat |> List.rev // Reverse to put them in the order they will be extracted later

[dsyme]

@alfonsogarciacaro I pushed two fixes to the visualfsharp PR that should I think address these problems

dotnet/fsharp@b7bb99d

[dsyme]

Let's get it green there then cherry-pick the fix-on-fix back here

[dsyme]

I checked most of the failing tests by hand and they seem to now be ok.

[alfonsogarciacaro]

Wow, you're fast! Ok, if everything is green in visualfsharp I can do the cherry-picking and send a new PR here if you want. Thanks a lot for your help!

[alfonsogarciacaro]

Previous failing tests now pass but there's a new failure 😞 I'm not sure how serious it is.

1) Failed : FSharp-Tests-Core+Load-Script.load-script
'C:\projects\visualfsharp-3dtit\tests\fsharp\core\load-script\out.stdout.txt' and 'C:\projects\visualfsharp-3dtit\tests\fsharp\core\load-script\out.stdout.bsl' differ; ["diff between [C:\projects\visualfsharp-3dtit\tests\fsharp\core\load-script\out.stdout.txt] and [C:\projects\visualfsharp-3dtit\tests\fsharp\core\load-script\out.stdout.bsl]";
 "line 10"; " - -the end"; " + Hello"]

[alfonsogarciacaro]

@dsyme, I tried rebuilding FCS applying your latest fix and unfortunately FSharpChecker.GetProjectOptionsFromScript doesn't seem to return the file names of any #load directive.

[dsyme]

Yeah, I know. I've made a better set of changes in dotnet/fsharp#1363, I think things are healthier now.

[dsyme]

Cherry picked the fix here. When both FCS and visualfsharp are green then we are in good shape