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

Add type annotations to entire function #1138

Merged
merged 6 commits into from
Jul 11, 2023

Conversation

nojaf
Copy link
Contributor

@nojaf nojaf commented Jul 10, 2023

Hi @Booksbaum and @baronfel,

I noticed there is no code fix for an entire let binding:
image

In Rider, I'm used to annotating the entire function (all parameters without types + return type).
I'd like to take a stab at adding this for fsac.

I think this is good enough to take it:
IonideAddTypesToFunction

@Booksbaum
Copy link
Contributor

Works already great 👍
And without directions :D


One issue I remember with FCS, FSharpMemberOrFunctionOrValue and ReturnParameter.Type (and tested in current FSAC too):
ReturnType of a function returning a lambda. Because FCS handles that in certain situation slightly ... different:

let f1 a = fun b -> a + b
let f2 a =
  ()
  fun b -> a + b

Both fs have signature int -> int -> int, and return type of fX a is int -> int. But FSharpMemberOrFunctionOrValue.ReturnParameter.Type is different for these two cases:

  • f1 has ReturnParameter.Type = int
  • f2 has ReturnParameter.Type = int -> int

-> f1 treats b as normal parameter (like a) -- while f2 returns a "normal" lambda.
(in AST: f1 Binding has a and b as SynValInfo -- while f2 only has a. Also visible in Tooltip: val f1: a: int -> b: int -> int vs val f2: a: int -> int -> int)

In code action:

let f1 (a: int) : int = fun b -> a + b
let f2 (a: int) : int -> int =
  ()
  fun b -> a + b

-> f1 has wrong type :/

I don't know how difficult it is to handle here in FSAC... But I don't think we have to handle that and instead treat it as bug in FCS? (don't know if that behaviour is by design or accident)


Handling LongIdent in InlayHints: would be great -- but I don't know how (re-)usable that would be in practice:

  • Inlay Hint might look ... strange:
    let f a = a + 1
    ->
    let f a:int :int = a + 1
    -> two : annotations, one for parameter one for function. And because just Inlay Hint there are no parens (though theoretical possible to show in hints too ... but then it's probably just too much clutter) I don't know how good or awful that would be in practice
  • I think the Code Fix would still have the same code you added -- even if we additional handle functions in InlayHints too: InlayHints works on individual identifiers, while CodeFix must handle more than one (function and parameters). So AST traversal is still necessary in Code Fix. (and for the rest you already use things from InlayHints)

@nojaf
Copy link
Contributor Author

nojaf commented Jul 11, 2023

Hey Booksbaum,

Thanks for all the pointers. I've had a similar issue with Telplin, so this edge case rings a bell.

Handling LongIdent in InlayHints: would be great -- but I don't know how (re-)usable that would be in practice

Alright, I think I want to pursue what I have for now and not overly worry about getting it inside Inlay Hints.

I tinker some more with this and poke you both when I could use a review.

@nojaf nojaf changed the title Showcase missing functionality Add type annotations to entire function Jul 11, 2023
@nojaf nojaf marked this pull request as ready for review July 11, 2023 20:04
@nojaf
Copy link
Contributor Author

nojaf commented Jul 11, 2023

Close enough 😸, ready for review!

@baronfel
Copy link
Contributor

This looks lovely - thank you for an exhaustive test suite as well. It's very easy to take fixes when they are of such a high quality!

@baronfel baronfel merged commit 4aaff04 into ionide:main Jul 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants