Fix error in optimization of for loops over strings and lists

[robertpi]

We recently discovered this issue in FAKE:
fsprojects/FAKE#985

Which actually comes from FAKE's dependency on FSharp.Compiler.Service, which is based on the latest version of visualfsharp, which contains the actual bug :)

The problem is that the optimization looks for code of the form:
let s = "to loop over"
for x in s do
System.Consolve.WriteLine(x)

This works when code loops over the string, as might be expected, but if you declare a string before the for loop and loop over something else the optimization is triggered and produces a bizarre compile error: For example the following program
let configure () =
let aSequence = seq { yield "" }
let aString = new System.String([||])
for _ in aSequence do
System.Console.WriteLine(aString)

configure ()

Give the following compile error:
myrepo.fs(5,65): error FS0971: Undefined value 'aString: string'

This pull request fixes the issue by looping at the expression that GetEnumator is called on, rather than the let declaration before the loop.

[msftclas]

Hi @robertpi, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!

In order for us to evaluate and accept your PR, we ask that you sign a contribution license agreement. It's all electronic and will take just minutes. I promise there's no faxing. https://cla.microsoft.com.

TTYL, MSBOT;

[forki]

Is this intended?

[robertpi]

Yes and no. It's unrelated to the fix, but I think it's needed as the compiler won't build from scratch without it. Seems the old package is no longer available on nuget. It's in a seperate commit.

[dsyme]

Perhaps submit this as a separate PR

[KevinRansom]

Thank you for the fix, could you please add a test so we don't regress this. Thanks

[robertpi]

Not sure what I should add as a test. The test I was going to add, to check the foreach loop is correctly turned into a for loop are covered in:
fsharpqa\Source\Optimizations\ForLoop\ForEachOnList01.fs
fsharpqa\Source\Optimizations\ForLoop\ForEachOnString01.fs

Tell me what else is needed and I'll add it.

[msftclas]

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

Thanks, MSBOT;

[KevinRansom]

Well it was discovered because of this: This works when code loops over the string, as might be expected, but if you declare a string before the for loop and loop over something else the optimization is triggered and produces a bizarre compile error: For example the following program ...

So I suppose the test is to ensure this compiles and runs correctly.

[KevinRansom]

According to appveyor the fsharp smoketests fail.

[00:27:42] Error: 'RunTests.cmd release fsharpqa Smoke' failed 

More output:

+++ CodeGen\EmittedIL\Misc (ForLoop01.fs - NetFx40) +++
[00:27:42] Microsoft (R) F# Compil
[00:27:42] er version (private)
[00:27:42] Copyright (c) Microsoft Corporation. All Rights Reserved.
[00:27:42] POSTCMD: [..\CompareIL.cmd ForLoop01.exe NetFx40]
[00:27:42] 
[00:27:42] C:\projects\visualfsharp-radou\tests\fsharpqa\Source\CodeGen\EmittedIL\Misc>REM == ForLoop01.exe --> assembly 
[00:27:42] 
[00:27:42] C:\projects\visualfsharp-radou\tests\fsharpqa\Source\CodeGen\EmittedIL\Misc>REM == NetFx40 --> NetFx20|NetFx40 (default is NetFx20) - case insensitive 
[00:27:42] 
[00:27:42] C:\projects\visualfsharp-radou\tests\fsharpqa\Source\CodeGen\EmittedIL\Misc>ildasm /TEXT /LINENUM /NOBAR "ForLoop01.exe"  1>"ForLoop01.il" 
[00:27:42] 
[00:27:42] C:\projects\visualfsharp-radou\tests\fsharpqa\Source\CodeGen\EmittedIL\Misc>IF NOT ERRORLEVEL 0 exit 1 
[00:27:42] 
[00:27:42] C:\projects\visualfsharp-radou\tests\fsharpqa\Source\CodeGen\EmittedIL\Misc>IF /I "NetFx40" == "NetFx40" goto :NetFx4 
[00:27:42] 
[00:27:42] C:\projects\visualfsharp-radou\tests\fsharpqa\Source\CodeGen\EmittedIL\Misc>..\..\..\..\testenv\bin\ILComparer.exe "ForLoop01.il.netfx4.bsl" "ForLoop01.il" 
[00:27:42] Files differ at line 49:
[00:27:42]  >>              [2] int32 wi,
[00:27:42]  <<              [2] class [FShar
[00:27:42] p.Core]Microsoft.FSharp.Collections.FSharpList`1<int32> V_2,
[00:27:42] 
[00:27:42] C:\projects\visualfsharp-radou\tests\fsharpqa\Source\CodeGen\EmittedIL\Misc>exit /b 1 
[00:27:42] 

[dsyme]

I've provided an alternative implementation of a fix here: #756 . It incorporates some of the ideas from this fix but applies some additional constraints. I've also added the original repro as a test.

[dsyme]

Closing as covered by 742

[dsyme]

A not-so-pretty workaround for this issue is here: #759 (comment). (note however it requires changes to user code)