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

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

Open
ploeh opened this issue Jun 18, 2023 · 7 comments
Open
Labels
Area-Diagnostics mistakes and possible improvements to diagnostics Feature Improvement
Milestone

Comments

@ploeh
Copy link
Contributor

ploeh commented Jun 18, 2023

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
Copy link
Member

T-Gro commented Jun 19, 2023

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
Copy link
Member

auduchinok commented Jun 19, 2023

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
Copy link
Member

auduchinok commented Jun 19, 2023

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
Copy link
Member

T-Gro commented Jun 19, 2023

@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
Copy link
Contributor Author

ploeh commented Jun 19, 2023

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
Copy link
Contributor

0101 commented Jun 19, 2023

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.

@0101 0101 added Area-Diagnostics mistakes and possible improvements to diagnostics and removed Area-Compiler-Syntax lexfilter, indentation and parsing labels Jun 19, 2023
@TheAngryByrd
Copy link
Contributor

Yeah this was a small detail I brought up in fsharp/fslang-design#706 (comment) so this applies to more than just casting.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Diagnostics mistakes and possible improvements to diagnostics Feature Improvement
Projects
Status: New
Development

No branches or pull requests

5 participants