Skip to content

Some restrictions for Parameter Hints - #935

Merged
baronfel merged 12 commits into
ionide:mainfrom
Booksbaum:InlayHints
May 1, 2022
Merged

baronfel merged 12 commits into
ionide:mainfrom
Booksbaum:InlayHints

Conversation

@Booksbaum

Copy link
Copy Markdown
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
Copy Markdown
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.

Comment thread src/FsAutoComplete.Core/InlayHints.fs Outdated
Comment thread src/FsAutoComplete.Core/InlayHints.fs Outdated
Comment thread test/FsAutoComplete.Tests.Lsp/InlayHintTests.fs
Comment thread test/FsAutoComplete.Tests.Lsp/InlayHintTests.fs Outdated
@baronfel

baronfel commented May 1, 2022

Copy link
Copy Markdown
Contributor

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