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

Some restrictions for Parameter Hints #935

Merged
merged 12 commits into from
May 1, 2022
Merged

Conversation

Booksbaum
Copy link
Contributor

(mostly inspired by rust-analyzer)

Parameter Hints are now additional hidden when:

  • Operator
    • In practice: were already not shown -- but because of short names, not because of operator
  • Param or Argument Prefix or Postfix of each other
    • prev: only same or arg starts with param

    • now: a bit more elaborated:

      • must be identifier (-> excludes other expressions like (foo |> bar) or foo.ToString())
        • Special case: extracts identifier from parens ((foo) -> foo)
      • only looks at most right identifier when long ident (foo.bar.baz -> baz)
        let f range = ()
        f data.Range
        //     ^^^^^ -> no param hint
        • to restrictive? arg might be config.baz.value -> baz in center
      • Removes leading _ from param for comparison
        let f _range = ()
        let range = 42
        f range
        //^^^^^ -> no param hint
      • Removes trailing ' from argument for comparison
        let f range = ()
        let range' = 42
        f range'
        //^^^^^^ -> no param hint
      • Pre/Postfix must be at word boundary (-> Upper case letter: foo is prefix of fooBar but not foobar)
        let f1 exactRange = ()
        //          ^ word boundary
        let f2 exactrange = ()
        //          ^ no word boundary
        let (range, exact) = (42, 13)
        
        f1 range
        // ^^^^^ -> no param hint
        f2 range
        // ^^^^^ -> param hint
        
        f1 exact
        // ^^^^^ -> no param hint
        f2 exact
        // ^^^^^ -> param hint
        • Besides at word boundary capitalization isn't considered: range = Range = RANGE
        • I'm not sure word boundaries are always good? might be too restrictive? or recognize something like plural s too?
        • All Pre/Postfix combinations (param pre/postfix of arg, arg pre/postfix of param) -> to intrusive? might allow to many names
        • Doesn't look what is inside word. But might be the match: arg=exactRangeOfExpr param=range -> might be nice to not show hint
        • Recognize acronym? f abstractSyntaxTree -> param ast -> no param hint?

      But: far to many possibilities when looking for something similar -> here now just some and not too hard to detect

  • Param postfix of Function Name:
    let validateRange range = ()
    //          ^^^^^ ^^^^^ -> never param hint
    • Like above: Postfix -> word boundary (-> uppercase)
  • Some more hiding when well known function. Unlike wellKnownNames (which just checks param name), I'll look at the function (and its module) too:
    -> only hide certain parameter names in certain functions:
    • option & voption in Option.xxx and ValueOption.xxx functions
    • format in printf functions
      • -> removed from general wellKnownNames: I think that's quite often a very valuable info -- just not for the commonly used printf-functions
    • mapping in Collection functions (List.xxx, Array.xxx, ...)
      • -> removed from general wellKnownNames

About "excludes expressions":
Might actually be worth recognizing some expressions (like param name is foo, arg: bar.Foo(), bar |> toFoo, ...) -- but there are so many possibilities -> I just completely ignore anything not-identifier.

Note: There are quite a few TODOs in tests for cases I'm not sure the current way is best. Any opinions?




Next:
I think we should move to Inlay Hints from LSP (coming in 3.17.0). Besides being an official LSP feature instead of custom one, this provides some additional things:

  • TextEdit[] instead of NewText: string option (inserted at Pos): A single insert at one position might not be enough to insert a type hint (might need parens)
  • Tooltip: Displaying a parameter's type and/or its XML Doc would be nice

If OK, I would add LSP types and corresponding request to FSAC

  • LSP Types here and not in ionide/LanguageServerProtocol: Inlay Hints still in preview -> easier to manage everything directly here instead of syncing with other repo (-> move once it's released)
  • Current fsharp/inlayHints stays (currently used in Ionide) (-> version of textDocument/inlayHint with simpler return data)

Add additional Test function to use positions marked in source
Prev: error when collecting expecto tests -> outside of tests
Now: error when running corresponding test -> inside a test
Example:
func: `validateRange`; param: `range`
Unlike `isNotWellKnownName`, not just by param name, but function (and its module/namespace) too.

I removed params `mapping` & `format` from common well known names,
and just hide them when used with a collection (`List.xxx`, `Array.xxx` functions) or one of the printf functions.
@baronfel
Copy link
Contributor

I haven't done a full review, but I did start porting the 3.17 types over at my fork of the LSP repo. That needs to be rebased now. I did this in part to get the InlayHint structures :D I don't see a reason not to start + implement both here in prep for the 3.17 release, though :) I agree about the need for the textEdit[] for sure! Then once VSCode supports 3.17 we can remove the fsharp/inlayHints endpoint entirely.

@baronfel
Copy link
Contributor

baronfel commented May 1, 2022

Thank you! This is again excellent work :) I'll re-release FSAC and include this in the new Ionide release today.

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.

2 participants