fix problem with loop optimization (#742) by dsyme · Pull Request #756 · dotnet/fsharp

dsyme · 2015-12-03T15:58:23Z

Provides a modified and reviewed fix for the problem reported here: #742

I've made the code cleaner and added additional constraints to ensure the optimization only applies to the compiled forms of for ... in ... do ... loops

robertpi · 2015-12-03T16:21:52Z

Cool. Looks good. I did wonder if checking the value in the enumerator and previous let binding was possible, but thought what I had was good enough. Would never have thought to make the change in the type checker either.

Apply dotnet/fsharp#756

brodyberg · 2015-12-04T04:49:09Z

In lieu of GitHub not lasting forever, one line of context would be helpful

KevinRansom · 2015-12-04T23:04:20Z

Thanks for fixing this merged.

dsyme · 2015-12-05T20:25:57Z

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

KevinRansom · 2015-12-06T03:52:42Z

Don is this right?

Yes, I think so - is there something I've missed? The "bodyExpr" is the body of the while loop, but it shouldn't directly refer to either enumerableVar (the variable holding the enumerable collection, which is consumed in GetEnumeratorCall) or enumeratorVar (the variable holding the enumerator for the collection, which is consumed in the binding for the elemVar "let").

KevinRansom · 2015-12-06T16:57:27Z

On the internal build this fails with:

D:\fsharp_D14\src\fsharp\projects\Prototype.FSharp.Compiler\FSharp.Compiler.fsproj" (BuildLinked target) (43:63) ->
CoreCompile target) ->
 D:\fsharp_D14\src\fsharp\devdiv\src\fsharp\tastops.fs(7768,23): error FS0010: Unexpected identifier in expression. Expecte
 'in' or other token. [D:\fsharp_D14\src\fsharp\projects\Prototype.FSharp.Compiler\FSharp.Compiler.fsproj]
 D:\fsharp_D14\src\fsharp\devdiv\src\fsharp\tastops.fs(7769,23): error FS0058: Possible incorrect indentation: this token i
 offside of context started at position (7767:23). Try indenting this token further or using standard formatting convention
. [D:\fsharp_D14\src\fsharp\projects\Prototype.FSharp.Compiler\FSharp.Compiler.fsproj]
 D:\fsharp_D14\src\fsharp\devdiv\src\fsharp\tastops.fs(7769,23): error FS0058: Possible incorrect indentation: this token i
 offside of context started at position (7767:23). Try indenting this token further or using standard formatting convention
. [D:\fsharp_D14\src\fsharp\projects\Prototype.FSharp.Compiler\FSharp.Compiler.fsproj]
 D:\fsharp_D14\src\fsharp\devdiv\src\fsharp\tastops.fs(7772,9): error FS0058: Possible incorrect indentation: this token is
offside of context started at position (7767:23). Try indenting this token further or using standard formatting conventions
 [D:\fsharp_D14\src\fsharp\projects\Prototype.FSharp.Compiler\FSharp.Compiler.fsproj]
 D:\fsharp_D14\src\fsharp\devdiv\src\fsharp\tastops.fs(7773,9): error FS0058: Possible incorrect indentation: this token is
offside of context started at position (7772:9). Try indenting this token further or using standard formatting conventions.
[D:\fsharp_D14\src\fsharp\projects\Prototype.FSharp.Compiler\FSharp.Compiler.fsproj]
 D:\fsharp_D14\src\fsharp\devdiv\src\fsharp\tastops.fs(7775,9): error FS0058: Possible incorrect indentation: this token is
offside of context started at position (7773:9). Try indenting this token further or using standard formatting conventions.
[D:\fsharp_D14\src\fsharp\projects\Prototype.FSharp.Compiler\FSharp.Compiler.fsproj]
 D:\fsharp_D14\src\fsharp\devdiv\src\fsharp\tastops.fs(7776,9): error FS0058: Possible incorrect indentation: this token is
offside of context started at position (7775:9). Try indenting this token further or using standard formatting conventions.
[D:\fsharp_D14\src\fsharp\projects\Prototype.FSharp.Compiler\FSharp.Compiler.fsproj]
 D:\fsharp_D14\src\fsharp\devdiv\src\fsharp\tastops.fs(7777,9): error FS0058: Possible incorrect indentation: this token is
offside of context started at position (7776:9). Try indenting this token further or using standard formatting conventions.
[D:\fsharp_D14\src\fsharp\projects\Prototype.FSharp.Compiler\FSharp.Compiler.fsproj]
 D:\fsharp_D14\src\fsharp\devdiv\src\fsharp\tastops.fs(7783,1): error FS0058: Possible incorrect indentation: this token is
offside of context started at position (7777:9). Try indenting this token further or using standard formatting conventions.
[D:\fsharp_D14\src\fsharp\projects\Prototype.FSharp.Compiler\FSharp.Compiler.fsproj]
 D:\fsharp_D14\src\fsharp\devdiv\src\fsharp\tastops.fs(7791,1): error FS0058: Possible incorrect indentation: this token is
offside of context started at position (7783:1). Try indenting this token further or using standard formatting conventions.
[D:\fsharp_D14\src\fsharp\projects\Prototype.FSharp.Compiler\FSharp.Compiler.fsproj]
 D:\fsharp_D14\src\fsharp\devdiv\src\fsharp\tastops.fs(7793,1): error FS0058: Possible incorrect indentation: this token is
offside of context started at position (7783:1). Try indenting this token further or using standard formatting conventions.
[D:\fsharp_D14\src\fsharp\projects\Prototype.FSharp.Compiler\FSharp.Compiler.fsproj]
 D:\fsharp_D14\src\fsharp\devdiv\src\fsharp\tastops.fs(7800,5): error FS0010: Incomplete structured construct at or before
his point in definition. Expected incomplete structured construct at or before this point or other token. [D:\fsharp_D14\sr
\fsharp\projects\Prototype.FSharp.Compiler\FSharp.Compiler.fsproj]
 D:\fsharp_D14\src\fsharp\devdiv\src\fsharp\tastops.fs(7868,1): error FS0058: Possible incorrect indentation: this token is
offside of context started at position (7793:1). Try indenting this token further or using standard formatting conventions.
[D:\fsharp_D14\src\fsharp\projects\Prototype.FSharp.Compiler\FSharp.Compiler.fsproj]````
The OSS build succeeds though.

mexx · 2015-12-06T22:01:56Z

fails for me also in the wild ;) already on the FSharp.Compiler-proto.dll

enricosada · 2015-12-07T13:45:31Z

why this difference with appveyor build?

mexx · 2015-12-07T15:22:56Z

I think it's a matter of what fsc version is used to build the proto compiler, will check which version was used in my setup.

mexx · 2015-12-07T23:21:28Z

Here is the start of the failing call on my setup
..\..\..\lkg\FSharp-4.0.30319.1\bin\fsc.exe -o:obj\Proto\net40\FSharp.Build-proto.dll ...
on AppVeyor
C:\Program Files (x86)\Microsoft SDKs\F#\3.1\Framework\v4.0\fsc.exe -o:obj\Proto\net40\FSharp.Build-proto.dll

KevinRansom · 2015-12-08T01:05:52Z

the appveyor build uses F# 3.1, that is the difference between build success and failure. It fails with the LKG.

mexx · 2015-12-08T06:11:30Z

What are the reasons why AppVeyor build doesn't use the LKG version, like the local ones?
Shouldn't the build take the same path as local ones?

KevinRansom · 2015-12-09T02:44:07Z

It'seems a bug in the script, we are looking at fixing it.

KevinRansom · 2015-12-09T06:34:40Z

pulled

fix problem with loop optimization

261732c

msftclas added the cla-not-required label Dec 3, 2015

dsyme changed the title ~~fix problem with loop optimization~~ fix problem with loop optimization (#742) Dec 3, 2015

dsyme mentioned this pull request Dec 3, 2015

Fix error in optimization of for loops over strings and lists #742

Closed

dsyme mentioned this pull request Dec 3, 2015

Repo for bug that affects FAKE fsharp/fsharp-compiler-docs#453

Closed

dsyme added a commit to dsyme/FSharp.Compiler.Service that referenced this pull request Dec 3, 2015

Apply dotnet/fsharp#756

fa91ca2

This was referenced Dec 3, 2015

Apply https://github.com/Microsoft/visualfsharp/pull/756/ fsharp/fsharp-compiler-docs#483

Merged

fix problem with loop optimization fsharp/fsharp#527

Merged

dsyme added a commit to fsharp/fsharp-compiler-docs that referenced this pull request Dec 3, 2015

Merge pull request #483 from dsyme/fix-opt-bug

049389b

Apply dotnet/fsharp#756

dsyme mentioned this pull request Dec 3, 2015

F# compiler bug in release mode in VS2015 Update 1 #759

Closed

brodyberg reviewed Dec 4, 2015
View reviewed changes

Update test.fsx

bb24268

KevinRansom added the Resolution-Fixed label Dec 4, 2015

KevinRansom closed this Dec 4, 2015

KevinRansom reviewed Dec 6, 2015
View reviewed changes

KevinRansom reopened this Dec 6, 2015

msftclas added the cla-not-required label Dec 6, 2015

mexx added a commit to mexx/visualfsharp that referenced this pull request Dec 6, 2015

fix build problem of dotnet#756

a989745

mexx mentioned this pull request Dec 6, 2015

Fix build problem of #756 #768

Merged

KevinRansom pushed a commit that referenced this pull request Dec 8, 2015

fix build problem of #756

52d4e00

KevinRansom pushed a commit that referenced this pull request Dec 8, 2015

fix build problem of #756

0ee2036

KevinRansom closed this Dec 9, 2015

dsyme added a commit to fsharp/fsharp-compiler-docs that referenced this pull request Dec 14, 2015

Apply dotnet/fsharp#756

a906a23

dsyme added a commit to fsharp/fsharp-compiler-docs that referenced this pull request Dec 14, 2015

Fix bug in loop optimization, apply dotnet/fsharp#756

7814c44

Uh oh!

Conversation

dsyme commented Dec 3, 2015

Uh oh!

robertpi commented Dec 3, 2015

Uh oh!

brodyberg Dec 4, 2015

Choose a reason for hiding this comment

Uh oh!

KevinRansom commented Dec 4, 2015

Uh oh!

dsyme commented Dec 5, 2015

Uh oh!

KevinRansom Dec 6, 2015

Choose a reason for hiding this comment

Uh oh!

dsyme Dec 6, 2015

Choose a reason for hiding this comment

Uh oh!

KevinRansom commented Dec 6, 2015

Uh oh!

mexx commented Dec 6, 2015

Uh oh!

enricosada commented Dec 7, 2015

Uh oh!

mexx commented Dec 7, 2015

Uh oh!

mexx commented Dec 7, 2015

Uh oh!

KevinRansom commented Dec 8, 2015

Uh oh!

mexx commented Dec 8, 2015

Uh oh!

KevinRansom commented Dec 9, 2015

Uh oh!

KevinRansom commented Dec 9, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants