[WIP] Allow _.Property shorthand for accessor functions

[tboby]

Just experimenting with _.foo.bar - > fun x - > x.foo.bar

Edit by @vzarytovskii: link to suggestion - https://github.com/fsharp/fslang-suggestions/issues/506

[vzarytovskii]

I've renamed it and added link to the suggestion, hope you don't mind @tboby

[tboby]

I think this is now a working proof of concept that implements the minimum features described in the suggestions issue.

Things I'm pretty sure need improving:

• [x] The way a conflicting underscore is detected for the diagnostic (performance + check starts with _ is a safe assumption)
• [x] Find a better name for the syntax tree object
• [x] Proper language server features (exposing enough for hover/etc)
• [x] Improve the walking of the tree to push the arg down into the shorthand
• [x] Confirm the parser rule is OK (the %prec is what worked, I didn't reason it out)
• [x] Add to language feature flags

[vzarytovskii]

@tboby please also make sure that the feature is under a language feature flag.

[vzarytovskii]

This also needs an RFC

[abelbraaksma]

This is pretty darn awesome! If you need some help with RFC, ping me on Slack or @-mention me here.

[T-Gro]

Hi @tboby, I really like this idea.

Would you mind if I try to have a look and attempt to revive this PR? Or are you planning to continue on it?

[tboby]

Hi @tboby, I really like this idea.

Would you mind if I try to have a look and attempt to revive this PR? Or are you planning to continue on it?

Absolutely, go ahead! I keep meaning to try and continue but haven't found the time.

Use as much of this and/or the RFC as is useful!

[T-Gro]

I spent some time trying to support _.prop in pattern matching, if user code supplies its own active pattern to handle it. However, the needed syntax constructs are not possible when in a match clause, so I am giving up.

(even a regular lambda (fun x -> x.Prop) leads to a parse error, as it is unexpected in matching context.

[T-Gro]

For completeness:

I spent some time trying to support _.prop in pattern matching, if user code supplies its own active pattern to handle it. However, the needed syntax constructs are not possible when in a match clause, so I am giving up.

(even a regular lambda (fun x -> x.Prop) leads to a parse error, as it is unexpected in matching context.

For full completeness, this is the syntax which remains non-supported: https://github.com/fsharp/fslang-suggestions/issues/968#issuecomment-854047531

let (|Member|_|) f = function null -> None | x -> Some <| f x
match typeof<int> with
| Member _.BaseType (Member _.BaseType null) -> printfn "A"
...

[auduchinok]

Are we in the right direction with the syntax tree by having yet another SynExpr to capture something with dots? Should we not start unifying SynExpr.DotGet, SynExpr.DotIndexedGet, SynExpr.LongIdent at some point?

It seems a new union case for _ in SynExpr would work nice, and it could be used as a qualifier in DotGet expressions.

[nojaf]

It seems a new union case for _ in SynExpr would work nice, and it could be used as a qualifier in DotGet expressions.

Yes, played around with that in https://github.com/dotnet/fsharp/pull/15548 The drawback of course is that the SynExpr.Wild on itself doesn't really mean anything. Although overall, it does feel a lot closer to the existing constructs.

[T-Gro]

@nojaf:

I think that the existing constructs are "undecided" on the granularity as well - doBang, matchBang exist as a more meaningful standalone SynExpr, instead of having a meaning-less (on its own) SynExpr.Bang. => I think the case could be made either way here.

For the spaces: I personally did not want to support spaces here. The poor-mans explanation of what's allowed following _. (very roughly, but 'explain to a 5 year old' style) is an expression without any spaces. So just a chain of primitive expressions, potentially separated by dots.

If by a different approach it would start to work, I would actually vote for not allowing _.ToString ()

[T-Gro]

Agree to the rest, will add the cases you mentioned so that it is visible already in this PR diff on how the tree looks like.

[nojaf]

I think that the existing constructs are "undecided" on the granularity as well - doBang, matchBang exist as a more meaningful standalone SynExpr, instead of having a meaning-less (on its own) SynExpr.Bang.

I guess it is very questionable whether these really had their own right to exist. From a syntax point of view, they can be captured in their non-bang counterpart.

At the same time, the updated index syntax (arr[0]) did not receive a special AST node. So I'm pretty sure you can find examples on both sides of this.

If by a different approach, it would start to work, I would actually vote for not allowing _.ToString ()

I very much would like to see that on record in the RFC. Because this will pop up eventually as a bug report on Fantomas' side.

Should an application with a SynExpr.DotLambda produce a warning if it is decided that it is illegal?

[T-Gro]

I have added:

• sigdata roundtrip test
• syntaxtree tests for positive and negative scenarios (some of the negative ones do not end up with an DotLambda syntraxtree item eventually)
• New negative tests for incorrect feature usage (puting spaces before "()" or using curried function call)

I am open to suggestions on how to:

• Spot the incorrect feature usage, especially where to handle it best
• Texting to change, the current messages in the test assertions are just the out of the box messages in case nothing gets changed.

[T-Gro]

The opening suggestion would be to analyze the inner synExpr (of the DotLambda node) in CheckExpressions and look for NonAtomic App . If one is found , report it.

Something like: " Underscore-dot lambda shorthand allows method calls with arguments in parenthesis, not separated by a space from the member name".

[T-Gro]

I think that the existing constructs are "undecided" on the granularity as well - doBang, matchBang exist as a more meaningful standalone SynExpr, instead of having a meaning-less (on its own) SynExpr.Bang.

I guess it is very questionable whether these really had their own right to exist. From a syntax point of view, they can be captured in their non-bang counterpart.

At the same time, the updated index syntax (arr[0]) did not receive a special AST node. So I'm pretty sure you can find examples on both sides of this.

If by a different approach, it would start to work, I would actually vote for not allowing _.ToString ()

I very much would like to see that on record in the RFC. Because this will pop up eventually as a bug report on Fantomas' side.

Should an application with a SynExpr.DotLambda produce a warning if it is decided that it is illegal?

I think the technical differentiation will be in not allowing NonAtomic App nodes (unless someone/something proves me wrong in that, it is just my initial guess).

The error message for end users must of course use a different vocabulary, I am open for suggestions here.

[vzarytovskii]

Yeah, we should (for now) disallow spaces and add note it in pr. Support for spaces and partial application (for untupled method) might be an additional feature.

[T-Gro]

/azp run

[azure-pipelines[bot]]

Azure Pipelines successfully started running 2 pipeline(s).

[T-Gro]

The diagnostics for incorrect usages updated and reflected in tests:

• ambigous usage of _
• using non-atomic expressions after _.