Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unnecessary conversion from 'YieldOrReturn' to 'YieldOrReturnFrom' #339

Closed
zakaluka opened this issue Oct 24, 2018 · 7 comments
Closed

Unnecessary conversion from 'YieldOrReturn' to 'YieldOrReturnFrom' #339

zakaluka opened this issue Oct 24, 2018 · 7 comments

Comments

@zakaluka
Copy link
Contributor

Per @jindraivanek's request, I am splitting off the second issue from #315.

Second question is about when I format through the Fantomas library and FAKE. I am seeing the following error message when I try to run the Server.fs file through checkCode.

D:\projects\ZenCase\src\Client\Client.fs:
Unable to cast object of type 'YieldOrReturn' to type 'YieldOrReturnFrom'.

D:\projects\ZenCase\src\Server\Server.fs:
Unable to cast object of type 'YieldOrReturn' to type 'YieldOrReturnFrom'.

The following files aren't formatted properly:
- D:\projects\ZenCase\src\Client\AssemblyInfo.fs
- D:\projects\ZenCase\src\Client\Client.fs !
- D:\projects\ZenCase\src\Migration\AssemblyInfo.fs
- D:\projects\ZenCase\src\Migration\Constants.fs
- D:\projects\ZenCase\src\Migration\Migration.fs
- D:\projects\ZenCase\src\Server\AssemblyInfo.fs
- D:\projects\ZenCase\src\Server\Server.fs !
- D:\projects\ZenCase\src\Shared\Shared.fs
- D:\projects\ZenCase\src\Shared\Utility.fs)

If I comment out all the computation expressions in Server.fs, then that error goes away. However, any computation express in Server.fs causes the error to be printed out. Because of this, I'm concerned about running Server.fs through formatCode.

I even tried leaving only one computation expression Server.fs:

let getInitCounter() : Task<Counter> = task { return 42 }

Based on what I saw in Fantomas, this is a YieldOrReturn. I still get the same error message. Why is Fantomas trying to change this to YieldOrReturnFrom (aka return!)?


@jindraivanek confirmed some additional details:

  1. I can confirm this, @zakaluka please create seperate issue with this snippet. It doesn't happen for each CE, my guess is it is connected to multiline expr as first thing in CE.
  2. Also confirmed. It can be reproduced wtih this FAKE5 script:
#r "paket: nuget Fantomas"
#load ".fake/test-FAKE5.fsx/intellisense.fsx"

let mkTempFile source =
    let fileName = System.IO.Path.GetTempFileName() + ".fs"
    System.IO.File.WriteAllText(fileName, source)
    fileName

let source = "let getInitCounter() : Task<Counter= task { return 42 }"

let path = mkTempFile source

Fantomas.CodeFormatter.FormatDocument(path, System.IO.File.ReadAllText(path), Fantomas.FormatConfig.FormatConfig.Default)

System.IO.File.Delete path

It's weird because with CLI this code will be formatted without problem.

@zakaluka
Copy link
Contributor Author

From the investigation, it appears that a mismatch in FSharp.Compiler.Service versions is to blame.

@jindraivanek
Copy link
Contributor

Thanks @zakaluka. I reiterate here:

I think we have two options how to fix this:

  1. upgrade FSharp.Compiler.Service to 25.0.1. That will force everyone who is using Fantomas as lib to also upgrade. Not sure if that is OK.
  2. change FSharp.Compiler.Service dependency in nuget to be exactly 23.0.3 (now is >=23.0.3). That would probably needs to change way of how we are creating nuget package, I wasn't able to find a way how to do it by dotnet pack parameter. It should be doable with paket pack.

I would prefer option 1.

@7sharp9, @auduchinok do you see any problem with Fantomas upgade to FSharp.Compiler.Service 25.0.1?

@auduchinok
Copy link
Contributor

@jindraivanek No, no problem at all on our side.
We have to repack both FCS and Fantomas so they reference the same FSharp.Core we ship anyways. We build Fantomas against our FCS fork that's updated from time to time.

@nojaf nojaf mentioned this issue Oct 27, 2018
@nojaf
Copy link
Contributor

nojaf commented Nov 20, 2018

@zakaluka the new release is out, could you confirm the problem is solved and close the issue if so?

@zakaluka
Copy link
Contributor Author

@nojaf I am traveling for the next few weeks, but will try to test this as soon as I am able. Unfortunately, the code I was testing on is on a desktop I do not currently have access to. I will see if I can just create a new project and test the results there.

@nojaf
Copy link
Contributor

nojaf commented Nov 26, 2018

Hi @zakaluka, no worries. Life happens. Thanks for replying and following this up.

@zakaluka
Copy link
Contributor Author

zakaluka commented Dec 8, 2018

@nojaf I've confirmed that the issue is not occurring any longer. Closing.

@zakaluka zakaluka closed this as completed Dec 8, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants