Inlay hints/PatternTypeHintsStage: support other patterns #756

DedSec256 · 2024-10-31T14:50:39Z

FCS update

auduchinok · 2024-11-07T11:50:02Z

@DedSec256 Do you think it could make sense to introduce another option, so hints for bindings/parameter declarations could be controlled separately? With both currently defaulting to 'push-to-hint' it doesn't make much difference, but I'm also trying to imagine if somebody could want to make the declaration hints be always shown while keep the 'match' ones in push-to-hint mode.

DedSec256 · 2024-11-07T23:55:07Z

ReSharper.FSharp/test/data/features/daemon/typeHints/Signatures - Top level 01.fs.gold

@@ -41,34 +41,34 @@ let g||(15) = fun x -> ()

 type A(x||(16)) =
    do
-        let x||(17) = 3


Accidentally fixed the case when hints showed for some local bindings even if it was disabled

DedSec256 · 2024-11-07T23:57:24Z

Perhaps, we can name it Match clause patterns

auduchinok · 2024-11-08T07:19:15Z

Perhaps, we can name it Match clause patterns

"Clause" sounds like a rather technical term to me. I've never heard it from non-compiler people. Something like "patterns" or "branches" may sound much more user-friendly here, I think?

auduchinok · 2024-12-03T12:42:42Z

ReSharper.FSharp/test/data/features/daemon/typeHints/Signatures - Locals 01.fs.gold

@@ -130,6 +130,21 @@ let (|String|) (x: MyStruct) : string = String(x.myString)

 let f1 (x & Int(i) & String(s)) = ()

+
+let _ = function x -> ()


What do you think about splitting this test files into smaller ones, so it could be easier to think about unrelated cases separately?

ReSharper.FSharp/test/data/features/daemon/typeHints/Signatures - Locals 01.fs.gold

auduchinok · 2024-12-03T12:45:05Z

ReSharper.FSharp/test/data/features/daemon/typeHints/Signatures - Match clauses 01.fs.gold

+        member _.Dispose(): unit = ()
+
+let delimiter (delim1, delim2, value) =
+    { new IFormattable with


Is it really a part of match-related test?

If the idea here is to test how different settings work, can these tests be simplified to only include minimal code needed for such tests?

I'd like to suggest we refactor these tests in a separate pull request.

Yes, sounds good. Maybe we should merge the refactor first then.

auduchinok · 2025-01-27T03:28:14Z

ReSharper.FSharp/src/FSharp/FSharp.Common/src/Settings/FSharpOptions.fs

+      [<SettingsEntry(PushToHintMode.PushToShowHints,
+                      DescriptionResourceType = typeof<Strings>,
+                      DescriptionResourceName = nameof(Strings.FSharpTypeHints_MatchPatterns_Description))>]
+      mutable ShowTypeHintsForMatchPatterns: PushToHintMode }


What's the idea behind having separate settings for patterns in local bindings and match clauses?

I think we discussed this question here #756 (comment), or was it about something else?

I think it wasn't about having a setting specifically about match patterns? I think we don't want to end up introducing separate settings for each place where patterns are allowed.

auduchinok · 2025-01-27T03:30:49Z

ReSharper.FSharp/src/FSharp/FSharp.Psi.Daemon/src/Stages/PatternTypeHintsStage.fs

-        let consumer = { TopLevelNodes = VisibilityConsumer(visibleRange, _.GetNavigationRange())
-                         LocalNodes = VisibilityConsumer(visibleRange, _.GetNavigationRange()) }
+        let consumer = {
+                         TopLevelNodes = VisibilityConsumer(visibleRange, _.GetNavigationRange())


Please reformat.

auduchinok · 2025-01-27T03:32:33Z

ReSharper.FSharp/src/FSharp/FSharp.Psi.Daemon/src/Stages/PatternTypeHintsStage.fs

@@ -181,7 +196,25 @@ type private PatternsHighlightingProcess(fsFile, settingsStore: IContextBoundSet
            createTypeHintHighlighting fcsType defaultDisplayContext range pushToHintMode actionsProvider false
            |> ValueSome

-        | _ -> ValueNone
+        | :? IParametersOwnerPat as pattern ->


Any reason for this change?

auduchinok · 2025-01-27T03:34:37Z

ReSharper.FSharp/test/data/features/daemon/typeHints/Patterns 02 - Match clauses.fs.gold

+    let y||(27) = 5 in ()
+
+match [[5]] with
+| [[]||(28)]||(29) -> ()


Do you think it makes sense to do it recursively here?

auduchinok · 2025-01-27T03:35:21Z

ReSharper.FSharp/test/data/features/daemon/typeHints/Patterns 02 - Match clauses.fs.gold

+| x :: _||(14) -> ()
+| _ :: x :: _||(15) -> ()
+| x :: _ :: _||(16) -> ()
+| x :: Some y||(17) :: _||(18) -> ()


Do you think it makes sense to do it recursively here?

I think for such cases I would like to see annotations for inner patterns as well

All the inferred types are going to be shown in the outer hint anyway, though. I'm not sure if it's helpful to show it for non-generic fields types. While it might help somewhat to see the types next to the patterns, I think about how to prevent such hints from becoming too noisy.

auduchinok · 2025-01-27T03:37:23Z

ReSharper.FSharp/test/data/features/daemon/typeHints/Settings 01 - Show all.fs.gold

    do
-        let x||(12) = 3
+        let x||(16) = 3


@DedSec256 This seems unrelated to match clauses. Could you split these test cases, please?

This is a special test case where all hints are enabled and contains the minimum set of each hint category. Previously I split tests into smaller ones here
313392c

DedSec256 marked this pull request as ready for review October 31, 2024 15:01

DedSec256 force-pushed the ber.a/matchClausePatterns branch from 3297ca0 to 5ce6bfb Compare November 7, 2024 23:26

DedSec256 commented Nov 7, 2024

View reviewed changes

DedSec256 requested a review from auduchinok November 8, 2024 13:36

auduchinok reviewed Dec 3, 2024

View reviewed changes

DedSec256 force-pushed the ber.a/matchClausePatterns branch from 21e091f to 38d2c7e Compare December 7, 2024 22:54

DedSec256 force-pushed the ber.a/matchClausePatterns branch from 38d2c7e to c476a56 Compare December 16, 2024 19:39

DedSec256 requested a review from auduchinok December 16, 2024 20:51

auduchinok reviewed Jan 27, 2025

View reviewed changes

DedSec256 force-pushed the ber.a/matchClausePatterns branch from c476a56 to a5717e3 Compare February 12, 2025 14:01

DedSec256 changed the title ~~Inlay hints/PatternTypeHintsStage: support match clause~~ Inlay hints/PatternTypeHintsStage: support other patterns Feb 12, 2025

DedSec256 force-pushed the ber.a/matchClausePatterns branch from a5717e3 to 098f0f5 Compare February 12, 2025 14:17

fixes

b802973

DedSec256 force-pushed the ber.a/matchClausePatterns branch from 91179eb to b802973 Compare February 12, 2025 15:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Inlay hints/PatternTypeHintsStage: support other patterns #756

Inlay hints/PatternTypeHintsStage: support other patterns #756

DedSec256 commented Oct 31, 2024 •

edited

Loading

auduchinok commented Nov 7, 2024

DedSec256 Nov 7, 2024 •

edited

Loading

DedSec256 commented Nov 7, 2024 •

edited

Loading

auduchinok commented Nov 8, 2024 •

edited

Loading

auduchinok Dec 3, 2024

auduchinok Dec 3, 2024

DedSec256 Dec 3, 2024

auduchinok Dec 3, 2024

auduchinok Jan 27, 2025

DedSec256 Feb 11, 2025

auduchinok Feb 11, 2025

auduchinok Jan 27, 2025

auduchinok Jan 27, 2025

auduchinok Jan 27, 2025

auduchinok Jan 27, 2025

DedSec256 Feb 11, 2025

auduchinok Feb 11, 2025

auduchinok Jan 27, 2025

DedSec256 Feb 11, 2025

		@@ -130,6 +130,21 @@ let (\|String\|) (x: MyStruct) : string = String(x.myString)

		let f1 (x & Int(i) & String(s)) = ()


		let _ = function x -> ()

Inlay hints/PatternTypeHintsStage: support other patterns #756

Are you sure you want to change the base?

Inlay hints/PatternTypeHintsStage: support other patterns #756

Conversation

DedSec256 commented Oct 31, 2024 • edited Loading

auduchinok commented Nov 7, 2024

DedSec256 Nov 7, 2024 • edited Loading

Choose a reason for hiding this comment

DedSec256 commented Nov 7, 2024 • edited Loading

auduchinok commented Nov 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DedSec256 commented Oct 31, 2024 •

edited

Loading

DedSec256 Nov 7, 2024 •

edited

Loading

DedSec256 commented Nov 7, 2024 •

edited

Loading

auduchinok commented Nov 8, 2024 •

edited

Loading