A code refactoring for explicit return type annotation

[psfinaki]

Given the code
let add x y = x + y

Create a refactoring that would suggesting rewriting it to
let add x y : int = x + y

I think this should be easy. We have:

1. Machinery to create refactorings
2. Methods to extract return type info - used in return type hints

Originally posted by @psfinaki in #10421 (comment)

[0101]

Possibly also for the parameters.

[SebastianAtWork]

Hi, I currently have a bit of free time and would like to get startet contributing to fsharp. If its ok with you guys, I would start looking into this. Any more pointers for a newcomer to this repo are of course appreciated :)

[psfinaki]

Hello there, @SebastianAtWork, you are absolutely very welcome to help with this :)

Here are a few tips for you:

• We already have a couple of refactorings here, take a look at what's going on there. Ultimately they all inherit from the dedicated VS class (in this case CodeRefactoringProvider) and override it's members.
• This refactoring we have is actually very close as it's adding type to the parameters. It's getting type information from the document and adds it whenever applicable.
• Unfortunately, we don't have a proper testing framework for this yet (as opposed to hints or code fixes). This means we'll need to test this manually for now. Here are the guidelines how to do that.

If you have any questions, feel free to ask. This should be doable and will be very useful to have!

[SebastianAtWork]

@psfinaki Already 2 weeks wow, time sure flies fast. Quick Update: I looked into how refactorings (and for some reason codefixes) work. I have a little bit of trouble finding resources for the different lexing and parsing symbols. Here is my current version that sill needs a few things:

 [<ExportCodeRefactoringProvider(FSharpConstants.FSharpLanguageName, Name = "AddExplicitReturnType"); Shared>]
type internal AddExplicitReturnType [<ImportingConstructor>] () =
    inherit CodeRefactoringProvider()

    override _.ComputeRefactoringsAsync context =
        asyncMaybe {
            let document = context.Document
            let position = context.Span.Start
            let! sourceText = document.GetTextAsync() |> liftTaskAsync
            let textLine = sourceText.Lines.GetLineFromPosition position
            let textLinePos = sourceText.Lines.GetLinePosition position
            let fcsTextLineNumber = Line.fromZ textLinePos.Line

            let! ct = Async.CancellationToken |> liftAsync

            let! lexerSymbol =
                document.TryFindFSharpLexerSymbolAsync(position, SymbolLookupKind.Greedy, false, false, nameof (AddExplicitReturnType))

            let! parseFileResults, checkFileResults =
                document.GetFSharpParseAndCheckResultsAsync(nameof (AddExplicitReturnType))
                |> CancellableTask.start ct
                |> Async.AwaitTask
                |> liftAsync

            let! symbolUse =
                checkFileResults.GetSymbolUseAtLocation(
                    fcsTextLineNumber,
                    lexerSymbol.Ident.idRange.EndColumn,
                    textLine.ToString(),
                    lexerSymbol.FullIsland
                )

            let isValidMethodWithoutTypeAnnotation (funcOrValue: FSharpMemberOrFunctionOrValue) (symbolUse: FSharpSymbolUse) =
                let isLambdaIfFunction =
                    funcOrValue.IsFunction
                    && parseFileResults.IsBindingALambdaAtPosition symbolUse.Range.Start

                (not funcOrValue.IsValue || not isLambdaIfFunction)
                && not (funcOrValue.ReturnParameter.Type.IsUnresolved)
                && not (parseFileResults.IsTypeAnnotationGivenAtPosition symbolUse.Range.Start)

            match symbolUse.Symbol with
            | :? FSharpMemberOrFunctionOrValue as v when isValidMethodWithoutTypeAnnotation v symbolUse ->
                let typeString = v.FullType.FormatWithConstraints symbolUse.DisplayContext
                let title = SR.AddExplicitReturnTypeAnnotation()

                let! symbolSpan = RoslynHelpers.TryFSharpRangeToTextSpan(sourceText, symbolUse.Range)
                let test = "Hi"

                let getChangedText (sourceText: SourceText) =
                    let debugInfo = $"{sourceText} : {typeString} : {symbolSpan} {test}"
                    debugInfo
                    let sub = sourceText.ToString(symbolSpan)

                    let newSub =
                        sub.Replace("=", $":{v.ReturnParameter.Type.TypeDefinition.DisplayName}=")

                    sourceText.Replace(symbolSpan, newSub)

                let codeAction =
                    CodeAction.Create(
                        title,
                        (fun (cancellationToken: CancellationToken) ->
                            async {
                                let! sourceText = context.Document.GetTextAsync(cancellationToken) |> Async.AwaitTask
                                return context.Document.WithText(getChangedText sourceText)
                            }
                            |> RoslynHelpers.StartAsyncAsTask(cancellationToken)),
                        title
                    )

                context.RegisterRefactoring(codeAction)
            | _ -> ()
        }
        |> Async.Ignore
        |> RoslynHelpers.StartAsyncUnitAsTask(context.CancellationToken)

1. isValidMethodWithoutTypeAnnotation should represent the condition for the availability of this refactoring. Am I thinking correctly/ am I missing something?
2. symbolUse currently only gives methe position of the Identifier. How can I find the = Sign Position (I assume thats where I have to add return types?)

[SebastianAtWork]

I think i still have trouble setting up vs2022 with fsharp XD Been switching between editors lately and its always something else that bugs me

[vzarytovskii]

@psfinaki Already 2 weeks wow, time sure flies fast. Quick Update: I looked into how refactorings (and for some reason codefixes) work. I have a little bit of trouble finding resources for the different lexing and parsing symbols. Here is my current version that sill needs a few things:

 [<ExportCodeRefactoringProvider(FSharpConstants.FSharpLanguageName, Name = "AddExplicitReturnType"); Shared>]

type internal AddExplicitReturnType [<ImportingConstructor>] () =

    inherit CodeRefactoringProvider()



    override _.ComputeRefactoringsAsync context =

        asyncMaybe {

            let document = context.Document

            let position = context.Span.Start

            let! sourceText = document.GetTextAsync() |> liftTaskAsync

            let textLine = sourceText.Lines.GetLineFromPosition position

            let textLinePos = sourceText.Lines.GetLinePosition position

            let fcsTextLineNumber = Line.fromZ textLinePos.Line



            let! ct = Async.CancellationToken |> liftAsync



            let! lexerSymbol =

                document.TryFindFSharpLexerSymbolAsync(position, SymbolLookupKind.Greedy, false, false, nameof (AddExplicitReturnType))



            let! parseFileResults, checkFileResults =

                document.GetFSharpParseAndCheckResultsAsync(nameof (AddExplicitReturnType))

                |> CancellableTask.start ct

                |> Async.AwaitTask

                |> liftAsync



            let! symbolUse =

                checkFileResults.GetSymbolUseAtLocation(

                    fcsTextLineNumber,

                    lexerSymbol.Ident.idRange.EndColumn,

                    textLine.ToString(),

                    lexerSymbol.FullIsland

                )



            let isValidMethodWithoutTypeAnnotation (funcOrValue: FSharpMemberOrFunctionOrValue) (symbolUse: FSharpSymbolUse) =

                let isLambdaIfFunction =

                    funcOrValue.IsFunction

                    && parseFileResults.IsBindingALambdaAtPosition symbolUse.Range.Start



                (not funcOrValue.IsValue || not isLambdaIfFunction)

                && not (funcOrValue.ReturnParameter.Type.IsUnresolved)

                && not (parseFileResults.IsTypeAnnotationGivenAtPosition symbolUse.Range.Start)



            match symbolUse.Symbol with

            | :? FSharpMemberOrFunctionOrValue as v when isValidMethodWithoutTypeAnnotation v symbolUse ->

                let typeString = v.FullType.FormatWithConstraints symbolUse.DisplayContext

                let title = SR.AddExplicitReturnTypeAnnotation()



                let! symbolSpan = RoslynHelpers.TryFSharpRangeToTextSpan(sourceText, symbolUse.Range)

                let test = "Hi"



                let getChangedText (sourceText: SourceText) =

                    let debugInfo = $"{sourceText} : {typeString} : {symbolSpan} {test}"

                    debugInfo

                    let sub = sourceText.ToString(symbolSpan)



                    let newSub =

                        sub.Replace("=", $":{v.ReturnParameter.Type.TypeDefinition.DisplayName}=")



                    sourceText.Replace(symbolSpan, newSub)



                let codeAction =

                    CodeAction.Create(

                        title,

                        (fun (cancellationToken: CancellationToken) ->

                            async {

                                let! sourceText = context.Document.GetTextAsync(cancellationToken) |> Async.AwaitTask

                                return context.Document.WithText(getChangedText sourceText)

                            }

                            |> RoslynHelpers.StartAsyncAsTask(cancellationToken)),

                        title

                    )



                context.RegisterRefactoring(codeAction)

            | _ -> ()

        }

        |> Async.Ignore

        |> RoslynHelpers.StartAsyncUnitAsTask(context.CancellationToken)

1. isValidMethodWithoutTypeAnnotation should represent the condition for the availability of this refactoring. Am I thinking correctly/ am I missing something?

2. symbolUse currently only gives methe position of the Identifier. How can I find the = Sign Position (I assume thats where I have to add return types?)

Since it still ignores result, and runs as a non-generic task, asyncMaybe is not necessary, cancellableTask should be used instead.

[psfinaki]

@SebastianAtWork - happy for your progress!

1. Yeah, this looks correct in your code
2. I guess you can get by searching the source text, that's what we do in some code fixes, so this should look familiar, e.g. take a look here. Alternatively, you should be able to ask the compiler for the last parameter (and get its position then) and insert the text after it (keeping in mind there might be none).

As for the cancellable tasks - yeah we are moving to them and it would be nice to have the new refactoring on those rails already. If you get confused with them, feel free to ask or create the PR as is and we'll guide you through replacing the corresponding parts - but you can get some inspiration here or here.

[SebastianAtWork]

@SebastianAtWork - happy for your progress!

1. Yeah, this looks correct in your code
2. I guess you can get by searching the source text, that's what we do in some code fixes, so this should look familiar, e.g. take a look here. Alternatively, you should be able to ask the compiler for the last parameter (and get its position then) and insert the text after it (keeping in mind there might be none).

As for the cancellable tasks - yeah we are moving to them and it would be nice to have the new refactoring on those rails already. If you get confused with them, feel free to ask or create the PR as is and we'll guide you through replacing the corresponding parts - but you can get some inspiration here or here.

Hi,
I further researched your examples and have created a draft pull request. #16077

• So far, I try to use the = sign for an explicit return type. I will definetly also try your suggestion with the last parameter position and compare.
• I tried to include a simple exploration test that calls the refactoring. Its slow and currently does not actually trigger the Refactoring. What could I be missing? I can debug line 64

 do context.RegisterRefactoring(codeAction)

But the breakpoints in line 51 or 52 are not hit and nothing is actually done

 let! sourceText = context.Document.GetTextAsync(cancellationToken)
 let changedText = getChangedText sourceText

• As far as I can see there is no build in option CE right? Would make the optional stuff a lot prettier.

Otherwise I am still experimenting and refactoring, but these answers would further help :)

[SebastianAtWork]

I just found my missing piece -.- The code action also has to be applied to a workspace . makes sense.

let refactorProvider = AddExplicitReturnType()
let refactoringActions= new List<CodeAction>()

let mutable refactorContext =
    CodeRefactoringContext(document, span, (fun a -> refactoringActions.Add (a)), ct)


do! refactorProvider.ComputeRefactoringsAsync refactorContext

let! operations = refactoringActions[0].GetOperationsAsync(ct)

for operation in operations do
    operation.Apply (workspace,ct)

Im currently trying to understand these Workspaces. AdHocWorkspace seems promising.

[psfinaki]

Oh okay, I just opened the solution to play with your branch - good that you've figured that out.

Yeah workspaces, yet another VS construction basically. Take a look, but you don't need to dive deep into that, I think we should have all the required machinery in the RoslynHelpers and technically things like GetFsDocument should be already creating all this ad-hoc stuff.

[SebastianAtWork]

@psfinaki Thank you for also looking at it. Yep, just pushed a version with workspace handling from the RoslynHelpers Solution. However, i still cant find the "result" of my refactoring. The AdHocWorkspace doesnt persist changes, but shouldnt the Document inside the Workspace somewhere have these Changes? I also played around with TryApplyChanges on the workspace but that returns false. Feels very close to working but not quite XD

[psfinaki]

@SebastianAtWork soooo my guess is that you might be facing the same thing that brought us a few bugs with code fixes back in a day. Both Document.WithText and SourceText.WithChanges actually work in a "pure" manner - they create new instances or the document and source text respectively. So AFAIU you are passing the modified version to the VS machinery (so things work) but in tests you access the initial version.

And since this was the case with hints and code fixes, I ended up creating all those frameworks which decouple text changes from their application. We might need to do something similar here.

[SebastianAtWork]

@psfinaki Ahh makes sense. Then I´ll look into CodeFixTEstFramework and see what I can do / understand. I was looking at the SolutionId and the handling of Workspaces and did see the crossing of pure and impure domains XD

[psfinaki]

@SebastianAtWork thanks - it would be amazing if you manage to test this automatically. It will be an example for the other refactorings we have there. Pure/impure APIs, yeah this "shines" when writing VS-related code in F#. Adapters and wrappers are all we are left with. Good design pattern exercise :D

[SebastianAtWork]

@psfinaki I found a way of getting the changed Document :) RefactorTestFramework.fs:

  for action in refactoringActions do
      let! operations = action.GetOperationsAsync ct
      for operation in operations do
          let codeChangeOperation = operation :?> ApplyChangesOperation
          codeChangeOperation.Apply(workspace,ct)
          solution <- codeChangeOperation.ChangedSolution 
          ()


  let! changedText = solution.GetDocument(document.Id).GetTextAsync(ct)

I have to dynamic cast the CodeChangeOperation to ApplyChangesOperation and voilá , I get a changed solution. Obviously I have to check what other Types of CodeChangeOperations there are and if I can somehow generalize this (Fsharp has that feature where you deconstruct a method parameter based on a property name I think, so that could work).

Also this version works while debugging the extension :) correct preview and fix. But I can still repeatedly add the return type XD Which led me to a question:
Would it be ok to call it Toggle Explicit Return Type? I feel like this would add additional value and I already have all the information for that.

[psfinaki]

@SebastianAtWork thanks for creating the testing rails. In general, I'd say that the framework can do whatever magic as long as the tests themselves are clear and there's not much white box testing happening.

Would it be ok to call it Toggle Explicit Return Type? I feel like this would add additional value and I already have all the information for that.

That's an interesting idea. I think we can go with that, this will bring extra value indeed. For some reason, I don't feel for the word "toggle" in this particular context (also it fits technically), I'd rather have explicit "add return type annotation" / "remove return type annotation".

One more thought on this - we should keep in mind that sometimes removing the explicit return type annotation can change it (to more generic). For example: let f x : int = x. Although I expect this to be harmless in most cases, I wouldn't suggest this refactoring there, because by definition refactoring shouldn't change the behavior.

There can be more dangerous cases:

type A = { X:int }
type B = { X:int }

let f (i: int) : A = { X = i }

Here, removing : A will change the return type to B.

type A = { X:int }
type B = { X:int }

let f (i: int) : A = { X = i }

let a: A = f 42

Here, removing : A in the first case will lead to code not compiling.

Check these cases for your implementation. If they work (the refactoring is not offered), please add the corresponding tests. If they don't and you don't want to bother - that's okay, we can add the toggle logic later. If they don't and you want to make them work, you get extra karma points :)

[SebastianAtWork]

@psfinaki I have a general Fsharp question that Im unable to find any answer to. How do I find for example all Methods that have SourceText as one Parameter? There used to be the FSDN search but that doesnt work anymore since the fsdn website is down.