@gmpl, Thanks for signing the contribution license agreement so quickly! Actual humans will now validate the agreement and then evaluate the PR.

Thanks, MSBOT;

Super, super interesting, especially since the adjustments are small

Seeing these failures - not sure why - some changes in errors/warnings may be expected but there are other failures here too

1) Failed : FSharp-Tests-Core+Members+Basics.Basics(GENERATED_SIGNATURE)
Error running command 'D:\j\workspace\release_ci_pa---866fd2c3\tests\..\release\net40\bin\fsc.exe' with args ' -r:System.Core.dll --nowarn:20 --define:COMPILED --sig:tmptest.fsi tmptest.fs' in directory 'D:\j\workspace\release_ci_pa---866fd2c3\tests\fsharp\core\members\basics'. ERRORLEVEL 1 ERRORLEVEL 1
at NUnitConf.checkTestResult(Result`2 result) in D:\j\workspace\release_ci_pa---866fd2c3\tests\fsharp\nunitConf.fs:line 14

2) Failed : FSharp-Tests-Core+Members+Basics.Basics(FSI_FILE)
Error running command 'D:\j\workspace\release_ci_pa---866fd2c3\tests\..\release\net40\bin\fsiAnyCPU.exe' with args ' -r:System.Core.dll --nowarn:20 --define:INTERACTIVE --maxerrors:1 --abortonerror test.fsi test.fs' in directory 'D:\j\workspace\release_ci_pa---866fd2c3\tests\fsharp\core\members\basics'. ERRORLEVEL 1 ERRORLEVEL 1
at NUnitConf.checkTestResult(Result`2 result) in D:\j\workspace\release_ci_pa---866fd2c3\tests\fsharp\nunitConf.fs:line 14

3) Failed : FSharp-Tests-Core+Members+Basics.Basics(FSC_OPT_PLUS_DEBUG)
Error running command 'D:\j\workspace\release_ci_pa---866fd2c3\tests\..\release\net40\bin\fsc.exe' with args ' -r:System.Core.dll --nowarn:20 --define:COMPILED --optimize+ --debug -o:test--optplus--debug.exe -g test.fsi test.fs' in directory 'D:\j\workspace\release_ci_pa---866fd2c3\tests\fsharp\core\members\basics'. ERRORLEVEL 1 ERRORLEVEL 1
at NUnitConf.checkTestResult(Result`2 result) in D:\j\workspace\release_ci_pa---866fd2c3\tests\fsharp\nunitConf.fs:line 14

4) Failed : FSharp-Tests-Core+Members+Basics.Basics(AS_DLL)
Error running command 'D:\j\workspace\release_ci_pa---866fd2c3\tests\..\release\net40\bin\fsc.exe' with args ' -r:System.Core.dll --nowarn:20 --define:COMPILED --optimize -a -o:test--optimize-lib.dll -g test.fsi test.fs' in directory 'D:\j\workspace\release_ci_pa---866fd2c3\tests\fsharp\core\members\basics'. ERRORLEVEL 1 ERRORLEVEL 1
at NUnitConf.checkTestResult(Result`2 result) in D:\j\workspace\release_ci_pa---866fd2c3\tests\fsharp\nunitConf.fs:line 14

5) Failed : FSharp-Tests-Core+Members+Basics.BasicsHw(FSI_FILE)
Error running command 'D:\j\workspace\release_ci_pa---866fd2c3\tests\..\release\net40\bin\fsiAnyCPU.exe' with args ' -r:System.Core.dll --nowarn:20 --define:INTERACTIVE --maxerrors:1 --abortonerror test.fsx' in directory 'D:\j\workspace\release_ci_pa---866fd2c3\tests\fsharp\core\members\basics-hw'. ERRORLEVEL 1 ERRORLEVEL 1
at NUnitConf.checkTestResult(Result`2 result) in D:\j\workspace\release_ci_pa---866fd2c3\tests\fsharp\nunitConf.fs:line 14

6) Failed : FSharp-Tests-Core+Members+Basics.BasicsHw(FSC_OPT_PLUS_DEBUG)
Error running command 'D:\j\workspace\release_ci_pa---866fd2c3\tests\..\release\net40\bin\fsc.exe' with args ' -r:System.Core.dll --nowarn:20 --define:COMPILED --optimize+ --debug -o:test--optplus--debug.exe -g test.fsx' in directory 'D:\j\workspace\release_ci_pa---866fd2c3\tests\fsharp\core\members\basics-hw'. ERRORLEVEL 1 ERRORLEVEL 1
at NUnitConf.checkTestResult(Result`2 result) in D:\j\workspace\release_ci_pa---866fd2c3\tests\fsharp\nunitConf.fs:line 14

7) Failed : FSharp-Tests-Core+Members+Basics.BasicsHwMutrec(FSI_FILE)
Error running command 'D:\j\workspace\release_ci_pa---866fd2c3\tests\..\release\net40\bin\fsiAnyCPU.exe' with args ' -r:System.Core.dll --nowarn:20 --define:INTERACTIVE --maxerrors:1 --abortonerror test.fs' in directory 'D:\j\workspace\release_ci_pa---866fd2c3\tests\fsharp\core\members\basics-hw-mutrec'. ERRORLEVEL 1 ERRORLEVEL 1
at NUnitConf.checkTestResult(Result`2 result) in D:\j\workspace\release_ci_pa---866fd2c3\tests\fsharp\nunitConf.fs:line 14

8) Failed : FSharp-Tests-Core+Members+Basics.BasicsHwMutrec(FSC_OPT_PLUS_DEBUG)
Error running command 'D:\j\workspace\release_ci_pa---866fd2c3\tests\..\release\net40\bin\fsc.exe' with args ' -r:System.Core.dll --nowarn:20 --define:COMPILED --optimize+ --debug -o:test--optplus--debug.exe -g test.fs' in directory 'D:\j\workspace\release_ci_pa---866fd2c3\tests\fsharp\core\members\basics-hw-mutrec'. ERRORLEVEL 1 ERRORLEVEL 1
at NUnitConf.checkTestResult(Result`2 result) in D:\j\workspace\release_ci_pa---866fd2c3\tests\fsharp\nunitConf.fs:line 14

9) Failed : FSharp-Tests-Typecheck+Sigs.sigs
Error running command 'D:\j\workspace\release_ci_pa---866fd2c3\tests\fsharpqa\testenv\bin\diff.exe' with args 'neg45.err neg45.bsl normalize' in directory 'D:\j\workspace\release_ci_pa---866fd2c3\tests\fsharp\typecheck\sigs'. ERRORLEVEL 1 ERRORLEVEL 1
at NUnitConf.checkTestResult(Result`2 result) in D:\j\workspace\release_ci_pa---866fd2c3\tests\fsharp\nunitConf.fs:line 14

Thanks for your feedback @dsyme I had a quick look at those tests but was not able to follow the code, actually I have no clue what is being tested (and failing) there, the error messages doesn't tell that much. Since I have no time in the week I will have a deeper look in the coming weekend.

@gmpl You need to look at the test output logs in order to determine what actually failed. For example, for the first test, FSharp-Tests-Core+Members+Basics.Basics(GENERATED_SIGNATURE), the output is here (if you're wondering how I found that, it's FSharpNunit_Output.log from the Windows_NT_Release_ci_part2_Build CI results linked in this PR.

From that log, we can see that the line that caused the test to fail had this error:

tmptest.fs(1805,42): error FS0071: Type constraint mismatch when applying the default type 'DateTime' for a type inference variable. This expression was expected to have type
    'DateTime'    
but here has type
    'TimeSpan'     Consider adding further type constraints

Your changes result in a discrepancy between the inference algorithm's results and the generated signature file (notice that --sig:tmptest.fsi is used), which we can confirm for ourselves:

(* subtracting two DateTimes results in a TimeSpan *)
> let ts = DateTime.Now - DateTime.Now;;
val ts : TimeSpan = 00:00:00

(* this won't work *)
> ((DateTime.Now - DateTime.Now) : DateTime);;

  ((DateTime.Now - DateTime.Now) : DateTime);;
  -----------------^^^^^^^^^^^^

stdin(6,18): error FS0001: This expression was expected to have type
    DateTime    
but here has type
    TimeSpan    

(* subtracting a TimeSpan from a DateTime produces a DateTime *)
> ((DateTime.Now - ts) : DateTime);;
val it : DateTime = 2016/09/14 3:20:01 {Date = 2016/09/14 0:00:00;
                                        Day = 14;
                                        DayOfWeek = Wednesday;
                                        DayOfYear = 258;
                                        Hour = 3;
                                        Kind = Local;
                                        Millisecond = 52;
                                        Minute = 20;
                                        Month = 9;
                                        Second = 1;
                                        Ticks = 636094200010529801L;
                                        TimeOfDay = 03:20:01.0529801;
                                        Year = 2016;}

(* the inference result without this PR for line 1805: *)
let f2 (x:DateTime) y : DateTime = x - y;;
val f2 : x:DateTime -> y:TimeSpan -> DateTime

(* and line 1806: *)
> let g2 (x:DateTime) y : TimeSpan = x - y;;
val g2 : x:DateTime -> y:DateTime -> TimeSpan

It's possible that the signature generation code simply needs to be updated to mirror whatever changes you've made here, but that's just a guess.

BTW, hats off to you if you can solve this--this has been my biggest F# pain point for a good year or more now, as someone who relies heavily on FsControl (sorry, Don :)).

@drvink Thanks a lot for the explanation.
Before doing this pull request I simplified a lot my original code which was more invasive than this.
I'm afraid I didn't test it so well, my fault, I wish I had infinite time.
But the good news is that apparently my original code pass those tests, I think I oversimplified it and broke type inference in the last step.
So don't lose hope, I will need some more weekends to clean-up the working code or find what's wrong with this simplification ... but in the end it will work ;)

Now the adjustments are not that small if you look at the number of lines changed but in fact the logic change is still small, I just introduced an additional argument to pass the member constraint solution and apply it later, when all the inference equations are already in place.

Didn't you have a extra repro sample? Can you please add it as a test?

@forki Do you mean as a test constrained by the compile time? If so, can you point me to similar tests already in the solution?

@gmpl I think I meant the sample given in #351 - does your pr fix it as well? If so then please try to add it as a test.

@dsyme Another alternative is rather than adding an additional parameter to many functions, add a property into the Solver State to store the constraint solution until we commit it. That change will be quite small in terms of lines of code and function signatures. What do you think?

@gmpl I will try to run that experiment: #1550

@gmpl can you please merge #1550 into this PR? Then I can close it.

@forki Now I see you were referring to another issue, I was thinking in the other one but they were marked as duplicate. So yes I will have a look tonight and try to merge it. Please leave it open for today, thanks.

@forki Do you know why all test failed after merging your test?
I mean, I can see the error message, but I wonder why it was passing before, you just added code for that specific test.

Unable to find version '14.0.24516-Pre' of package 'VisualCppTools'.

that's nuget hickup, completely unrelatd to the merge.

@dotnet-bot test this please.

I wonder if it will have much difference on type checking during IDE, I guess it needs to be back ported to FCS to test.

AFAIK this PR speed ups the type checker specifically, so FCS should speed up even further as it does not do byte code generation.

hopefully parallelism can speed it up to the point where people stop complaining about how slow the compiler is 😸

@cloudRoutine for IDE? Yes, I hope. As for building, it's already parallized by msbuild/VS, so CPU is about 100% load.

@vasily-kirichenko no I mean parallelizing the actual process of typechecking

@cloudRoutine That would be very interesting, unfortunately I can only work on weekends.
Anyway I'm still not happy with the code, I think before going parallel we should make the solver algorithm more consistent.
I'm convinced it can speed up some more and it can also be smarter, there are many cases where it's not able to solve and should be possible.

@vasily-kirichenko I don't think this optimization changed the name resolution order, there were other important changes since F# 4.0 that most likely did that. Did you check with the previous version before this commit?

Nice to hear that your project compilation speeds up a lot, do you use FsControl or a do you use a lot of "constrained overloadeds" there?

I tried to compile FsControl, before it was taking 2' 30'' and now with this version it takes only 30 secs :) Anyway I think the FsControl.dll compilation is not the part that will benefit the most from this optimization.
The sample scripts should speed up more, I still have to test and compare them.

@gmpl @forki @vasily-kirichenko It's crucial that we determine where name resolution changed in F# 4.1 as soon as possible, and if possible revert it to previous behaviour.

@gmpl @vasily-kirichenko If there's any chance you could do a binary chop on the commits for F# 4.1 to determine where name resolution changed that would be awesome. It might be that using the commit history for http://github.com/fsharp/fsharp may be easier.

@forki Could you carefully check if your name resolution changes altered the order of name resolution results in cases like the above

@vasily-kirichenko can you come up with a repro case?

#1530 (comment)?

@vasily-kirichenko I tried to put that into a test: #1652

Looks like it resolves fine for me on master:

Maybe it behaves differently if files compiled as part of a project?

Does it work the same in F# 4 FSI?

@forki It might need to have the two namespace fragments in one file

Come to think of it, I think this may have been a deliberate bug fix, see this commit - I do recall fixing an inconsistency when combining namespace fragments from a single file. It might even have been that the effective combined declaration order flipped every time a "combine" happened.

If so I suppose we would be OK with this being a breaking change, you could always workaround by qualifying the union cases by the type name.

Tbh I think this case was always the same and I think the current behavior is correct. @vasily-kirichenko can you try to modify my sample so that it breaks for you?

@forki I tried to reproduce it on a minimal project and failed. Also I benchmarked 4.0/4.1-before-optimization/4.1-master compilers by compiling FCS project and all of them showed identical results (~42 seconds). Now I'm not sure why my large solution is compiled more than 2x faster on master compiler. I switched between 4.0 and master ones several times and it's reliably reproduced. Can it be some mess with ngen or the like?

@vasily-kirichenko if your solution doesn't make use of constrained overload resolution I don't see why it would speed up that much with this PR. But if it does, even in a relative small number it could have improved, since some cases were taking insane compile time.