Initial attempt at ReplaceReturnTypeFix. #367

nojaf · 2022-05-04T08:18:55Z

Hello @auduchinok, this is a first attempt to add a quick fix action for the Type constraint mismatch error when a defined return type differs from the actual type of an expression.

Please let me know what the best next steps could be here.
I imagine unit tests and edge cases.

auduchinok · 2022-05-10T11:26:44Z

ReSharper.FSharp/src/FSharp.Psi.Intentions/src/QuickFixes/ReplaceReturnTypeFix.fs

+    let rec visitParent (expr: IFSharpExpression) =
+        match expr.Parent with
+        | :? IFSharpExpression as parent -> visitParent parent
+        | _ -> expr


This approach doesn't seem to be correct. Consider examples like:

let f x : int = match () with | _ -> "" | _ -> 1 1

let f x : int = "" + 1 1

In both cases it'll go up to the binding despite that the error is not at the "return" position for that binding.
It should only go up if the expression is at the position that a type annotation refers to. For example, if an expression is inside a sequentialExpr we should only go up if it's the last expression. Or if it's an expression in let Pat = Expr1 in Expr2 we should only go up for Expr1 and not for Expr2.

RecordCtorReference.GetFcsSymbol does something similar (though, I'm not sure if it's complete enough). It should probably be extracted into some common util method.

ReSharper.FSharp/src/FSharp.Psi.Intentions/src/QuickFixes/ReplaceReturnTypeFix.fs

auduchinok

@nojaf Could you try to unify the tree traversal with the logic in RecordCtorReference.GetFcsSymbol?
https://github.com/JetBrains/resharper-fsharp/blob/net222/ReSharper.FSharp/src/FSharp.Psi/src/Util/FSharpExpressionUtil.cs would be a good place to put it to.

ReSharper.FSharp/src/FSharp.Psi.Intentions/src/QuickFixes/ReplaceReturnTypeFix.fs

nojaf · 2022-05-17T15:41:02Z

@auduchinok, I'm slowly getting the hang of this.
Let me know where you still see gaps.
I'm sure there are some more cases regarding ITypeUsage.

auduchinok

@nojaf The overall approach is much better now, I left a few comments here and there.

ReSharper.FSharp/src/FSharp.Psi/src/Util/FSharpExpressionUtil.cs

ReSharper.FSharp/test/data/features/quickFixes/replaceReturnType/Replace type with attribute.fs

....FSharp/test/data/features/quickFixes/replaceReturnType/Replace type in infix application.fs

auduchinok · 2022-05-17T16:34:42Z

ReSharper.FSharp/src/FSharp.Psi.Intentions/src/QuickFixes/ReplaceReturnTypeFix.fs

+            | :? IFunctionTypeUsage as ftu ->
+                ftu.SetReturnTypeUsage(typeUsage) |> ignore


This probably needs to take more conditions into account. It seems for the examples below it'll only cover the first one:

let f1 _ : unit -> int = fun _ -> "" let f2 _ : unit -> int = id

ReSharper.FSharp/src/FSharp.Psi.Intentions/src/QuickFixes/ReplaceReturnTypeFix.fs

auduchinok

Here goes another round. 🙂

ReSharper.FSharp/src/FSharp.Psi/src/Util/FSharpExpressionUtil.cs

auduchinok · 2022-05-18T10:15:34Z

ReSharper.FSharp/src/FSharp.Psi/src/Util/FSharpExpressionUtil.cs

+          continue;
+        }
+
+        if (BinaryAppExprNavigator.GetByArgument(currentExpr) is { } binaryAppExpr)


We likely need to be more cautious here. Consider this:

let _: int = 1 + ""

I'd also check if the logic is the same for piping. For some reason I think there might be some quirks with binary operators and type conversions.

In general, it's usually better to not provide quick fix in cases were we aren't sure we work correctly than breaking the user code in an unexpected way. It's better to gradually enable cases later than to ship it as soon as possible and to deal with a situation where you can't easily fix all the corner cases and don't want to disable the feature too.

ReSharper.FSharp/src/FSharp.Psi/src/Util/FSharpExpressionUtil.cs

ReSharper.FSharp/test/src/FSharp.Intentions.Tests/src/QuickFixes/ReplaceReturnTypeTest.fs

auduchinok · 2022-05-19T11:24:18Z

ReSharper.FSharp/src/FSharp.Psi/src/Util/FSharpExpressionUtil.cs

+        if (TupleExprNavigator.GetByExpression(currentExpr) is { } tupleExpr)
+        {
+          currentExpr = tupleExpr;
+          continue;
+        }


Could you show an example where we need to go up from a tuple expression?

See test Tuple return type.fs

Thanks! Please check the samples below, they currently throw exceptions:

let x: int * string = 1 let x: int * string = if true then 1, "" else 1, 1

Another interesting case:

let x: int -> int = 1, 1

I think tuple (and probably lambda) expressions handling should be moved out of this method. Traversing them makes us go through parts of a type instead of going through inner expressions with the same overall type and may require keeping additional knowledge like "what item we came from", so it works in nested tuples and other complex cases. It also might be good to keep this method simple, so it could be easier to reuse it.

It's fine to move them to another method or to the quick fix code for now. Maybe we'll decide to move it back later.

We shouldn't go up here, but it's OK to go up in the quick fix (as you already seem to do).

auduchinok · 2022-05-19T11:26:20Z

ReSharper.FSharp/src/FSharp.Psi/src/Util/FSharpExpressionUtil.cs

+        if (LambdaExprNavigator.GetByExpression(currentExpr) is { } lambdaExpr)
+        {
+          currentExpr = lambdaExpr;
+          continue;
+        }


Likewise, can we go up from any lambda body?

See test FunctionType 01.fs.

I see, thanks. We should carefully consider possible syntax and FCS types here:

let f: int -> double -> string = Unchecked.defaultof<_> let g: int -> int -> string = f

let h: int * (int -> int) = 1, fun x -> ""

Both these cases don't work correctly with the current implementation. We should disable going up from lambda expressions for now.

auduchinok · 2022-05-19T11:27:38Z

ReSharper.FSharp/src/FSharp.Psi/src/Util/FSharpExpressionUtil.cs

+          continue;
+        }
+
+        if (BinaryAppExprNavigator.GetByArgument(currentExpr) is { } binaryAppExpr)


let x: int = 1 + ""

Yes, still on my to-do list.

Please remove this if for now 🙂

auduchinok · 2022-05-19T12:53:17Z

ReSharper.FSharp/src/FSharp.Psi.Services/src/Daemon/Highlightings/FcsErrors.xml

@@ -525,5 +525,18 @@
    <Behavour overlapResolvePolicy="NONE"/>
    <QuickFix>ReplaceReturnTypeFix</QuickFix>
  </Error>
-
+
+  <Error staticGroup="FSharpErrors" name="TypeMisMatchTuplesHaveDifferingLengths" ID="FS0001: Type mismatch. Expecting">


Maybe we probably should check this inside the quick fix's IsAvailable method instead of creating different error types. I'll need to think about it.

Parsing the error also seem a too ad-hoc and a bit dangerous, it would be much better to work with FCS's FSharpType instead. It's not always possible to get that view of the expected type, though.

nojaf · 2022-05-19T14:08:49Z

Would it be an option to put some more logic into the override this.IsAvailable member to skip the more complicated scenarios?

auduchinok · 2022-05-19T14:32:37Z

Yes, that sounds fine.

auduchinok

I left a few comments about cases that don't work as expected and that we should fix or disable. Otherwise, it seems to work great for many cases now!

auduchinok · 2022-05-24T11:25:18Z

ReSharper.FSharp/src/FSharp.Psi.Intentions/src/QuickFixes/ReplaceReturnTypeFix.fs

+            let returnType =
+                match binding.ReturnTypeInfo.ReturnType.IgnoreParentParens() with
+                | :? IParenTypeUsage as ptu -> ptu.InnerTypeUsage
+                | returnType -> returnType


Probably this is what you want here:

Suggested change

let returnType =

match binding.ReturnTypeInfo.ReturnType.IgnoreParentParens() with

| :? IParenTypeUsage as ptu -> ptu.InnerTypeUsage

| returnType -> returnType

let returnType = binding.ReturnTypeInfo.ReturnType.IgnoreInnerParens()

The current code takes the outermost type from the return node, then tries to go up through paren types as much as it could (but it's already at the top type usage, so it doesn't go anywhere) and then tries to go down through parens once. That doesn't look completely correct and might not work with nested paren types.

auduchinok · 2022-05-24T16:02:23Z

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

+            | Regex typeMisMatchTupleLengths [expectedType; actualType] ->
+                let expr = nodeSelectionProvider.GetExpressionInRange(fsFile, range, false, null)
+                TypeMisMatchTuplesHaveDifferingLengthsError(expectedType, actualType, expr, error.Message)


There's something wrong with reported types when item count mismatch:

let x: int * int * int = 1, ""

Let's disable this case until the error is fixed?

auduchinok · 2022-05-24T16:06:54Z

ReSharper.FSharp/src/FSharp.Psi.Intentions/src/QuickFixes/ReplaceReturnTypeFix.fs

+        let refPat = binding.HeadPattern.As<IReferencePat>()
+        if isNull refPat then () else
+
+        let symbolUse = refPat.GetFcsSymbolUse()
+        if isNull symbolUse then () else


I seems that we don't use the symbol right now (though, we probably should it future), so perhaps we don't need to require a specific kind of pattern for now.

auduchinok · 2022-05-24T16:10:41Z

ReSharper.FSharp/src/FSharp.Psi.Intentions/src/QuickFixes/ReplaceReturnTypeFix.fs

+        if isNotNull binding.ReturnTypeInfo
+           && isNotNull binding.ReturnTypeInfo.ReturnType then
+            let factory = binding.CreateElementFactory()
+            let typeUsage = factory.CreateTypeUsage(replacementTypeName)


I consider this creation of a type usage from a text parsed in an error text to be a bit dangerous but we can try to use it for the time being.

auduchinok · 2022-05-24T16:13:07Z

ReSharper.FSharp/src/FSharp.Psi.Intentions/src/QuickFixes/ReplaceReturnTypeFix.fs

+                match mostOuterParentExpr with
+                | :? ITupleExpr ->
+                    ModificationUtil.ReplaceChild(ftu, typeUsage) |> ignore
+                | _ -> ftu.SetReturnTypeUsage(typeUsage) |> ignore


This doesn't handle this case correctly:

let x = 1, 2 let f: int -> int = x

auduchinok · 2022-05-24T16:17:36Z

ReSharper.FSharp/src/FSharp.Psi/src/Util/FSharpExpressionUtil.cs

+    // TODO: change name
+    public static IFSharpExpression GetOuterMostParentExpression(this IFSharpExpression expression)


Maybe we use something like GetOutermostParentExpressionFromItsReturn for now (though, I find it too long for to be good).

auduchinok · 2022-05-24T16:19:03Z

ReSharper.FSharp/src/FSharp.Psi/src/Util/FSharpExpressionUtil.cs

+        if (LambdaExprNavigator.GetByExpression(currentExpr) is { } lambdaExpr)
+        {
+          currentExpr = lambdaExpr;
+          continue;
+        }


Both these cases don't work correctly with the current implementation. We should disable going up from lambda expressions for now.

auduchinok · 2022-05-24T16:20:05Z

ReSharper.FSharp/src/FSharp.Psi/src/Util/FSharpExpressionUtil.cs

+        if (TupleExprNavigator.GetByExpression(currentExpr) is { } tupleExpr)
+        {
+          currentExpr = tupleExpr;
+          continue;
+        }


We shouldn't go up here, but it's OK to go up in the quick fix (as you already seem to do).

auduchinok · 2022-05-24T16:20:35Z

ReSharper.FSharp/src/FSharp.Psi/src/Util/FSharpExpressionUtil.cs

+          continue;
+        }
+
+        if (BinaryAppExprNavigator.GetByArgument(currentExpr) is { } binaryAppExpr)


Please remove this if for now 🙂

nojaf · 2022-06-01T12:56:40Z

@auduchinok I've added some additional checks inside the quick fix to not enable it when the return type or expression cannot be processed in a reliable manner.
Let me know if this is what you had in mind and what to do with the corresponding tests.

…le' does not match the type 'int' as well.

auduchinok

@nojaf Thanks for this, this is a very nice quick fix to have!

nojaf · 2022-06-08T12:36:18Z

My pleasure, thank you for all the guidance along the way!

auduchinok reviewed May 10, 2022

View reviewed changes

nojaf force-pushed the replace-return-type branch from d2dbdda to d7d8519 Compare May 12, 2022 14:10

auduchinok reviewed May 12, 2022

View reviewed changes

ReSharper.FSharp/src/FSharp.Psi.Intentions/src/QuickFixes/ReplaceReturnTypeFix.fs Outdated Show resolved Hide resolved

nojaf force-pushed the replace-return-type branch from 8b1b57c to af9a74e Compare May 17, 2022 15:38

auduchinok reviewed May 17, 2022

View reviewed changes

auduchinok reviewed May 18, 2022

View reviewed changes

auduchinok reviewed May 19, 2022

View reviewed changes

auduchinok reviewed May 24, 2022

View reviewed changes

nojaf force-pushed the replace-return-type branch from f79fc38 to 67a8e7e Compare June 1, 2022 11:28

nojaf added 16 commits June 8, 2022 12:55

Initial attempt at ReplaceReturnTypeFix.

0fb183f

Enable ReplaceReturnTypeFix quickfix for error FS0001: The type 'doub…

4ddcfcb

…le' does not match the type 'int' as well.

Only visit parent of Sequential if expr is last item.

4aea04f

Allow fix when expression is body of SynMatchClause.

f2f3cfb

Add initial quick fix tests.

c421202

Add failing test to restore return type.

6f7df23

Only replace TypeUsage from ReturnType.

d4e2d3c

Refactor quick fix to use FSharpExpressionUtil.

adb9812

Add negative tests.

9a00e80

Add some more tests.

5c2de3e

Refactor GetOuterMostParentExpression to use Navigator helpers.

44aebcd

Add negative test for record field expression.

8ddf350

Remove For- and WhileExprNavigator.

5f5168e

Remove definition of FooAttribute.

7cd784e

Replace MatchExprNavigator with MatchClauseListOwnerExprNavigator.

58921d1

Rename and group unit tests.

abac5d5

nojaf and others added 16 commits June 8, 2022 12:55

Add test for LetOrUse.

23d1071

Add tests for IfThenElse.

678ed64

Accept the replacement type in the constructor of ReplaceReturnTypeFix.

31f1462

Add tests for function type return type.

b130065

Add newline to end of test data files.

40133b7

Parentheses around the return type.

516eae3

Tuple return type, where one type doesn't match.

e5b9609

Tuple return type, mismatch in number of items, to few in return type.

b395f15

Add test for too many items in tuple return type.

e76f69e

Don't assume type to replace a tuple with is also a tuple.

dba882e

Verify if the closest parent is a tuple expr to see if the items match.

f5ea03c

Replace function type with tuple.

153184b

Remove tuple and lambda support.

6197440

Mark tests as not available

b745a59

Renamed helper function GetOutermostParentExpressionFromItsReturn

fa7389a

Disable quick fix via highlighting registration

88396a2

auduchinok force-pushed the replace-return-type branch from 707abf0 to 88396a2 Compare June 8, 2022 12:30

auduchinok approved these changes Jun 8, 2022

View reviewed changes

nojaf marked this pull request as ready for review June 8, 2022 12:35

auduchinok merged commit 5a433da into JetBrains:net222 Jun 8, 2022

auduchinok pushed a commit that referenced this pull request Jun 8, 2022

Quick fixes: replace return type (#367)

c3ca234

		\| :? IFunctionTypeUsage as ftu ->
		ftu.SetReturnTypeUsage(typeUsage) \|> ignore

		// TODO: change name
		public static IFSharpExpression GetOuterMostParentExpression(this IFSharpExpression expression)

Initial attempt at ReplaceReturnTypeFix. #367

Initial attempt at ReplaceReturnTypeFix. #367

Conversation

nojaf commented May 4, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

auduchinok left a comment

Choose a reason for hiding this comment

nojaf commented May 17, 2022

auduchinok left a comment

Choose a reason for hiding this comment

auduchinok May 17, 2022 • edited

Choose a reason for hiding this comment

auduchinok left a 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

auduchinok May 19, 2022 • edited

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

nojaf commented May 19, 2022

auduchinok commented May 19, 2022

auduchinok left a 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

nojaf commented Jun 1, 2022

auduchinok left a comment

Choose a reason for hiding this comment

nojaf commented Jun 8, 2022

auduchinok May 17, 2022 •

edited

auduchinok May 19, 2022 •

edited