[regression] static member can be defined without member keyword, in unexpected way

[smoothdeveloper]

 type Seq =
   static meme average (x: int seq) = x |> Seq.map float |> Seq.average;;

notice I typed meme and not member.

It fails to parse in older fsharp, tried fsharpi, where it fails with:

error FS0010: Unexpected identifier in type definition

With current F#, it defines a static member meme that takes average: obj and x: int seq as input and returns float, which I can then call:

Seq.meme null [1..10];;

5.5

I don't think we want the meaning to change, and it should fail the same way it used to, unless I missed something.

[vzarytovskii]

Wondering if it's new recovery rules cc @auduchinok

[vzarytovskii]

Or more likely, new static members @T-Gro did?

[T-Gro]

Will take a look.

It is definitely not an F# 8 feature to add a meme keyword :]]

[T-Gro]

@auduchinok :

I have a feeling that this PR https://github.com/dotnet/fsharp/pull/15102/files#diff-e4c6a418b5f63918e99d3b27f209771a966fb6c7dbadf25cf5d3bb4b7b7d7bd7R1504 actually allowed it by design.

Or was the idea to then watch for SynLeadingKeyword.Static (as opposed to the correct SynLeadingKeyword.StaticMember during typechecking?

I could raise an error in the | STATIC rule, but that would feel like going against your original PR.

[auduchinok]

Wondering if it's new recovery rules cc @auduchinok

Yes, it's quite likely.

I have a feeling that this PR https://github.com/dotnet/fsharp/pull/15102/files#diff-e4c6a418b5f63918e99d3b27f209771a966fb6c7dbadf25cf5d3bb4b7b7d7bd7R1504 actually allowed it by design.

Definitely not by design, but it could come unintentionally from the changes to multiple rules at the same time. I'll try to take a look this week.

[auduchinok]

I could raise an error in the | STATIC rule, but that would feel like going against your original PR.

Parsing static only is needed for the parsing of unfinished declarations. Adding an error inside of it could work, but would probably not work as well in cases like the one above where there're subsequent tokens in the declaration, but also an error, as multiple possible unrelated errors would be reported for a single problem. Parsing meme as an identifier is not too bad here while typing, we just need (a better) error reporting.

I see two options to try from the top of my head:

• produce an error in the member rule when the parsed keywords are SynLeadingKeyword.Static
  This allows some custom error text but makes the recovery even a bit more artificial in this case (but it may be OK)
• change | STATIC into | STATIC recover
  There's a chance it'll just work and will also produce good error like expecting member or let or something like that. There's also a chance that something inside these other rules will influence the generation in some way (e.g. some precedences inside the rules would take over), and recovery simply won't work as expected.

The second option looks good, if it works with this simple change or something similar. If not, I'd probably try to go for the first one.

[T-Gro]

It's a good idea.
I have tried it and looks good - I will raise a PR to see the diff for existing recovery constructs.

[ncave]

Given that omitting the member keyword after static has been allowed in .NET 7.0 and .NET 8.0 (but not in .NET 6.0), this could be a breaking change for some code.

I wonder if it will be a good idea to continue to allow it to be optional by checking for static let/do/val, and defaulting to static member otherwise? It could be a bit more consistent with how the member keyword is optional after abstract (or not even allowed after override, default, etc.).

OTOH there is a good argument for allowing member to be optional only when it is unambiguously a member.

[T-Gro]

I don't think it was ever allowed to be omitted (except for this very mistake/issue) in the general case for 'static member'.
https://sharplab.io/#v2:DYLgZgzgNALiCWwA+MCeAHApgAgMqYEdsBeAWAChsrsIYBDGeAY2wFtNWAjTAJ2zoBuvOgHMcACgAeIbPAB2MGoQCUJbJOxIAfHkIA6VnXTYwwAPYNNO/AT2DhYoA===

Also I cannot find having 'static' (without member) as a new announced feature - it was really an accident caused by parser recovery improvements.

Nevertheless, for code written during this issue's existence, this might be a breaking change - but will be reported as a parser error, not cause a runtime failure.