dotnet / fsharp

The F# compiler, F# core library, F# language service, and F# tooling integration for Visual Studio
https://dotnet.microsoft.com/languages/fsharp
MIT License
3.87k stars 781 forks source link

FS3118 and FS0010 when combined with task CE and function name is too long #15432

Open ploeh opened 1 year ago

ploeh commented 1 year ago

Please accept my apologies if this is a known issue, or not reproducible.

Repro steps

Use Visual Studio to create a new .NET 6.0 F# class library. Modify the default Library.fs file by giving it this content:

module ClassLibrary1

open System.Threading.Tasks

let f2345 () = task {
    return () } :> Task

Expected behavior

The code compiles.

Actual behavior

Compilation fails with two error messages:

error FS0010: Unexpected symbol ':>' in binding. Expected incomplete structured construct at or before this point
or other token.

error FS3118: Incomplete value or function definition. If this is in an expression, the body of the expression must
be indented to the same column as the 'let' keyword.

Known workarounds

There are (at least) two workarounds to this problem:

  1. Make the function name shorter. With the given example, change the function name to f234, and the code compiles. The above code is a minimal repro of an issue I've run into more than once, and while I don't understand the exact relationship, the maximum function name length seems to be somehow related to the last expression in the task CE. (Here, it's just return (), but with a longer expression, the name can be longer, too.)
  2. Format the code like this:
module ClassLibrary1

open System.Threading.Tasks

let f2345 () =
    task {
        return () } :> Task

This alternative formatting seems to resolve the problem, and I haven't seen any problems with it.

This syntax does, however, introduce an extra line of code, and what is worse, an extra level of indentation, which I'd like to avoid.

Related information

If you're wondering about why anyone would want to upcast Task<'a> to Task this is only a minimal repro. My actual use case is asynchronous integration tests written with xUnit.net. The xUnit.net framework can execute Task-valued operations, but not Task<'a>-valued operations.

I suspect that the repro syntax may strictly speaking not be valid F# syntax, but usually it works. In any case it's confusing that it compiles when the function name is short, but stops compiling when the name exceeds a threshold.

I've discovered this issue while working with F# in Visual Studio (details below), but it also reproduces with dotnet build from the command line. I don't know if it reproduces in Visual Studio Code.

Edition Windows 11 Pro Version 22H2 Installed on ‎27.‎01.‎2023 OS build 22621.1848 Experience Windows Feature Experience Pack 1000.22642.1000.0

.NET 6

Microsoft Visual Studio Community 2022 Version 17.6.3 VisualStudio.17.Release/17.6.3+33801.468 Microsoft .NET Framework Version 4.8.09032

Installed Version: Community

Visual C++ 2022 00482-90000-00000-AA879 Microsoft Visual C++ 2022

ASP.NET and Web Tools 17.6.326.62524 ASP.NET and Web Tools

Azure App Service Tools v3.0.0 17.6.326.62524 Azure App Service Tools v3.0.0

Azure Functions and Web Jobs Tools 17.6.326.62524 Azure Functions and Web Jobs Tools

C# Tools 4.6.0-3.23259.8+c3cc1d0ceeab1a65da0217e403851a1e8a30086a C# components used in the IDE. Depending on your project type and settings, a different version of the compiler may be used.

CodeMaintainability 2022 1.2 Extension for tracking code maintainability of your methods. Instead of often performing report driven code analysis tools, you can use this extension to view in real-time maintainability score.

Common Azure Tools 1.10 Provides common services for use by Azure Mobile Services and Microsoft Azure Tools.

Microsoft JVM Debugger 1.0 Provides support for connecting the Visual Studio debugger to JDWP compatible Java Virtual Machines

NuGet Package Manager 6.6.0 NuGet Package Manager in Visual Studio. For more information about NuGet, visit https://docs.nuget.org/

Razor (ASP.NET Core) 17.6.0.2327201+a6a61fdfa748eaa65aab53dab583276e26af4a3e Provides languages services for ASP.NET Core Razor.

SQL Server Data Tools 17.6.13.0 Microsoft SQL Server Data Tools

Surrounder 1.0.12 Surrounds any selected texts with matching quotation or brace pairs

TypeScript Tools 17.0.20329.2001 TypeScript Tools for Microsoft Visual Studio

Visual Basic Tools 4.6.0-3.23259.8+c3cc1d0ceeab1a65da0217e403851a1e8a30086a Visual Basic components used in the IDE. Depending on your project type and settings, a different version of the compiler may be used.

Visual F# Tools 17.6.0-beta.23174.5+0207bea1afae48d9351ac26fb51afc8260de0a97 Microsoft Visual F# Tools

Visual Studio IntelliCode 2.2 AI-assisted development for Visual Studio.

T-Gro commented 1 year ago

Relevant: https://learn.microsoft.com/en-us/dotnet/fsharp/style-guide/formatting#avoid-formatting-that-is-sensitive-to-name-length

After playing with the sample a little ( https://sharplab.io/#v2:DYLgZgzgPgtg9gEwK7AKYAIDCwCGEIAyAlgEYBOOZAngIwCwAUI3AA6oB26AylRAC6oYAOgAqACzKocCIuwDmovAGsIjRmj7owAJgDMAFgCs6ABQBKdAF50fZegDejdM+eS+SMp3PoAvuhAAfOgiykA= ), I think the rule I am observing so far is:

When using the style of declaration, the number of spaces used for indentation must be higher or equal than the length of the identifier.

This is of course not good since it is not refactoring friendly, which is also why this style is not recommended in the F# style guide. And what does indeed cause a lot of confusion is that it only starts showing up when other constructs are chained on the same line (here :> Task), and not without them.

auduchinok commented 1 year ago

It's not related to the name length in any way, and the same error can be seen without the binding:

Screenshot 2023-06-19 at 11 21 40

The issue happens due to how indentation syntax works in F#. Normally, an operator requires at least the same indentation as its operands, but there's a special case allowing it to be less indented by its length + one space:

    1
  + 1
    ""
 :> obj

If the operator is indented less than that, then there's this parsing error:

    ""
:> obj

What makes the case in this issue harder to analyze is the fact the braces allow additional deindentation inside them too. That doesn't affect the subsequent code after them, and :> ends up being less indented than required by the context (seemingly started at {). The more idiomatic way to format it would probably be something like this:

module ClassLibrary1

open System.Threading.Tasks

let f2345 () =
    task {
        return ()
    } :> Task

In theory it should be possible to allow cases where a token could also have less indentation when continuing a deintented context before it, but it would require a change in the language, so in that case it would go to F# lang suggestions repo first and would be discussed and carefully designed there.

auduchinok commented 1 year ago

I found some interesting details while experimenting with this sample. It seems that when task is the first thing on a line, its offset is used as the indentation context start:

task {
    return ()
} :> Task
let f1 () =
    task             {
        return ()
    } :> Task // still allowed

But when it's not the first thing on the line (or indeed is placed as the binding body), then { offset seems to be the context start. It could in theory be a small bug somewhere in LexFilter. Or it could be a deliberate design choice because of some other issues or limitations, I don't know. 🙂

T-Gro commented 1 year ago

@auduchinok : The relation to the length of the function is indirect here - it affects where the context starts, since name length keeps moving the "{" character.

ploeh commented 1 year ago

Thank you for investigating. I'm not surprised that it seems to be fundamentally linked to my non-idiomatic formatting.

FWIW, my main reason for reporting this was that it confused me enough that I thought it might be helpful to others, if there was a place one could go to when running into this issue.

Even if Don Syme ultimately rules that my use of the language is invalid, I think the compiler errors are so cryptic that, if possible, better messages might be helpful.

0101 commented 1 year ago

Any changes to this behavior should go through language suggestions. See https://github.com/fsharp/fslang-design/blob/main/FSharp-6.0/FS-1108-undentation-frenzy.md for examples.

We might want to look into improving the error messages somehow.

TheAngryByrd commented 1 year ago

Yeah this was a small detail I brought up in https://github.com/fsharp/fslang-design/issues/706#issuecomment-1284657617 so this applies to more than just casting.