Open psfinaki opened 1 year ago
Possibly also for the parameters.
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 :)
Hello there, @SebastianAtWork, you are absolutely very welcome to help with this :)
Here are a few tips for you:
If you have any questions, feel free to ask. This should be doable and will be very useful to have!
@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)
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
@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)
isValidMethodWithoutTypeAnnotation should represent the condition for the availability of this refactoring. Am I thinking correctly/ am I missing something?
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.
@SebastianAtWork - happy for your progress!
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 - happy for your progress!
- Yeah, this looks correct in your code
- 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
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
Otherwise I am still experimenting and refactoring, but these answers would further help :)
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.
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.
@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
@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.
@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
@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
@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.
@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 :)
@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.
@SebastianAtWork hmm you mean in our source code base? Or in an arbitrary source code base at hand?
@SebastianAtWork hmm you mean in our source code base? Or in an arbitrary source code base at hand?
In general. I know some have the convention of having module names after the type with extra functionality. Is this true for the fsharp codebase or are there other conventions or tools to explore?
@SebastianAtWork I don't know to be sure. Nothing better than RegEx comes to my mind.
@SebastianAtWork, in case it is helpful, Resharper/Rider has "Find Usages Advanced..."
"Usages of member" could be of help, due to type inference, the type you look for may not show up, but usage of members will eventually bring the results of where the symbols are actually put to use.
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 https://github.com/dotnet/fsharp/issues/10421#issuecomment-1622467357