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

Allow _.Property / _.MethodCall() / _.IndexerAccess[idx] shorthand for accessor functions #13907

Merged
merged 55 commits into from
Jul 20, 2023

Conversation

tboby
Copy link
Contributor

@tboby tboby commented Sep 15, 2022

_.foo.bar- > fun x - > x.foo.bar

link to suggestion - fsharp/fslang-suggestions#506
RFC PR : fsharp/fslang-design#710

Allowed constructs

Treating _.expression as fun x -> x.expression


let a1 : {| Foo : {| Bar : string |}|} -> string = _.Foo.Bar
let a2 : {| Foo : int array |} -> int = _.Foo.[5]
let a3 : {| Foo : int array |} -> int = _.Foo[5]
let a4 : {| Foo : unit -> string |} -> string = _.Foo()
let a5 : {| Foo : int -> {| X : string |} |} -> string = _.Foo(5).X
let a6 = [1] |> List.map _.ToString()

Now with correct coloring and autocomplete:
image

And with right-hand-side derivation of the input type:
(not saying I would recommend such style of coding)
image

@vzarytovskii vzarytovskii changed the title [WIP] Prop shorthand [WIP] Allow _.Property shorthand for accessor functions Sep 15, 2022
@vzarytovskii
Copy link
Member

vzarytovskii commented Sep 15, 2022

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

@tboby
Copy link
Contributor Author

tboby commented Sep 24, 2022

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:

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

@vzarytovskii
Copy link
Member

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

@nojaf
Copy link
Contributor

nojaf commented Jul 5, 2023

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

T-Gro commented Jul 7, 2023

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

T-Gro commented Jul 7, 2023

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

T-Gro commented Jul 7, 2023

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

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

T-Gro commented Jul 10, 2023

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@T-Gro
Copy link
Member

T-Gro commented Jul 17, 2023

The diagnostics for incorrect usages updated and reflected in tests:

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

@T-Gro T-Gro requested a review from 0101 July 19, 2023 15:25
@T-Gro T-Gro marked this pull request as draft July 19, 2023 15:25
@T-Gro T-Gro marked this pull request as ready for review July 19, 2023 15:25
@T-Gro T-Gro requested a review from a team July 19, 2023 15:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

8 participants