QuickFix for ValueNotContainedMutabilityLiteralConstantValuesDiffer #546

dawedawe · 2023-07-07T10:41:55Z

Still very WIP, but this should update a [<Literal>] val in a signature file to be in sync with the impl.

Recording.2023-07-07.182925.mp4

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

auduchinok · 2023-07-07T19:18:31Z

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

+        if implMfv.FullType.BasicQualifiedName <> sigMfv.FullType.BasicQualifiedName then
+            let returnTypeString = implMfv.ReturnParameter.Type.Format(sigSymbolUse.DisplayContext)
+            let factory = sigRefPat.CreateElementFactory()
+            let typeUsage = factory.CreateTypeUsage(returnTypeString, TypeUsageContext.TopLevel)
+            sigRefPat.Binding.ReturnTypeInfo.SetReturnType(typeUsage) |> ignore


Please consider a type that doesn't exist in the signature file:

type E = | A = 1 [<Literal>] let a = E.A

Updating a signature would break code in the signature file.

auduchinok · 2023-07-07T19:20:33Z

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

+            let typeUsage = factory.CreateTypeUsage(returnTypeString, TypeUsageContext.TopLevel)
+            sigRefPat.Binding.ReturnTypeInfo.SetReturnType(typeUsage) |> ignore
+
+        sigRefPat.Binding.SetExpression(error.Pat.Binding.Expression.Copy()) |> ignore


Please consider code that doesn't exist in the signature file:

[<Literal>] let MyLiteral = 1 [<Literal>] let a = MyLiteral

Please also consider more complex literal expressions:

[<Literal>] let a = 1 + 1

And symbol from opened namespaces (that may be unopened in the signature):

open Module [<Literal>] let a = SymbolFromModule

open Ns [<Literal>] let a = Module.Symbol

Please also consider more complex literal expressions:

[<Literal>] let a = 1 + 1

Thanks for all the pointers. I made some progress with most cases, I think.
There's still a gap when checking if something like

[<Literal>] let a = 1 + SomeOtherLiteral

would be valid in the signature file.

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

auduchinok

@dawedawe I've left some comments. The overal approach looks good! There are many small things need tweaking or some design considerations that could've been done beforehand. Please feel free to ask for pointers when starting a feature like this one, it could make it easier. 🙂

...arper.FSharp/test/src/FSharp.Intentions.Tests/src/QuickFixes/UpdateLiteralConstantFixTest.fs

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

auduchinok · 2023-07-11T10:18:01Z

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

+    let tryFindSigBindingSignature sigMembers =
+        sigMembers
+        |>  Seq.tryPick(fun m ->
+            let bindingSignature = m.As<IBindingSignature>()


Is there a similar error produced for enum value declarations? If yes, we should try to make it easy to enable the same quick fix in all cases.

I'll have a look at that.

The error that is raised for enum values is also raised for record fields (FS193) and it's currently handled by UpdateRecordFieldTypeInSignatureFix. So I guess, we could expand that fix to also handle enums or merge it with the fix of this PR. Not sure yet, what's better.

auduchinok · 2023-07-11T10:19:42Z

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

+                    match bindingSignature.HeadPattern with
+                    | :? IReferencePat as sigRefPat when


Does it work with parens?

[<Literal>] let (a) = 1

Are there any other compound patterns that are allowed in a literal binding?

Good question. I can't think of one. Do you have another in mind?

I can't think of one.

Could you check it, please? I.e. look at what kind of patterns are possible to type there, then check if analysis allows them to be in a literal binding. Looking at FSharp.psi grammar will really help to see the possible options.

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

auduchinok

@dawedawe There's some good progress here! I've added a few more comments. 🙂

auduchinok · 2023-07-13T08:20:50Z

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

+    let mutable sigRefPat = null
+
+    let rec isImplExprValidInSig (implExpression: IFSharpExpression) =
+        match implExpression with


Please use implExpression.IgnoreInnerParens()

auduchinok · 2023-07-13T08:23:08Z

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

+    let rec isImplExprValidInSig (implExpression: IFSharpExpression) =
+        match implExpression with
+        | :? IReferenceExpr as refExpr ->
+            refExpr.Reference.ResolveWithFcs(sigRefPat, System.String.Empty, false, refExpr.IsQualified)


Please pass something like UpdateLiteralConstantFix.IsAvailable as the opName.

auduchinok · 2023-07-13T08:25:37Z

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

+    let rec isImplExprValidInSig (implExpression: IFSharpExpression) =
+        match implExpression with
+        | :? IReferenceExpr as refExpr ->
+            refExpr.Reference.ResolveWithFcs(sigRefPat, System.String.Empty, false, refExpr.IsQualified)


I think resolveExpr should be true. Resolve in expressions and other places use a bit different set of the rules (e.g. you can't type a type name as an expression), and we're checking if a reference expression is resolved, so expression rules sound like a correct thing to do.

auduchinok · 2023-07-13T08:27:25Z

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

+    let rec isImplExprValidInSig (implExpression: IFSharpExpression) =
+        match implExpression with
+        | :? IReferenceExpr as refExpr ->
+            refExpr.Reference.ResolveWithFcs(sigRefPat, System.String.Empty, false, refExpr.IsQualified)


@dawedawe Could you please clarify why qualified is set to refExpr.IsQualified? What is it supposed to achieve?

As I understand it, that's needed for cases like

type E = | A = 1 | B = 2 [<Literal>] let c = E.A

See test Update with enum case - 01
If we pass false for qualified, then inside of ResolveWithFcs the names parameter is set to just A.

Correct, the qualified resolve is required in this case. But in addition to that, if we look at the ResolveWithFcs implementation, nothing should break if you just always pass true.

auduchinok · 2023-07-13T08:29:46Z

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

+            |> Option.isSome
+        | :? IBinaryAppExpr as binExpr ->
+            isImplExprValidInSig binExpr.LeftArgument && isImplExprValidInSig binExpr.RightArgument
+        | _ -> true


We should probably be more defensive here. It's probably better to allow literal expressions and disallow the most of the other cases. What I mean is if there's some other compound syntax that is allowed in literal bindings, then we may skip the check if names are still resolved in the signature. Could you check if any other syntax is allowed?

While still being a ILiteralExpr, there are also measures.
As we can't use ResolveWithFcs to check if the measure types are valid in the signature, I think we should not support them.

While still being a ILiteralExpr, there are also measures.

That's a really good observation! It seems our grammar lacks them for some reason. We should fix it, and the check will become easy to do. 🙂

auduchinok · 2023-07-13T08:34:17Z

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

+open JetBrains.ReSharper.Psi.Tree
+open JetBrains.ReSharper.Resources.Shell
+
+type UpdateLiteralConstantFix(error: LiteralConstantValuesDifferError) =


Thanks for simplifying the names! They sound a bit too ambiguous now, though. 🙂 For example, we can later have a quick fix updating the implementation side, or even some unrelated quick fix.
Let's call it something like UpdateSignatureLiteralConstantFix or UpdateLiteralConstantInSignatureFix?

The same mostly applies to the error name too.

dawedawe · 2023-07-27T11:17:45Z

Instead of disabling the QuickFix for UoM constants, we could also check if they are valid in the sig file.
I tried that in the last commit. There's still work to do if the UoM type itself needs an update, too.
Let me know, if you're interested in supporting them or if I should revert.

…antValuesDiffer

update signature, not impl

…exact same name as in FSComp.txt

- handle symbol availability from opened modules

- refactor to use ResolveWithFcs() - support patterns with parens

…ibute

- pass opName to ResolveWithFcs - pass true as resolveExpr to ResolveWithFcs - pass true as qualified to ResolveWithFcs

…eFix" rename folder of test files

…luesDifferInSignature"

…y broken for UoM

dawedawe · 2023-08-23T10:46:23Z

Because of dotnet/fsharp#15843 we can't update the return type for UoM Literals but otherwise I think the UoM support in this CodeFix is okay now.

auduchinok reviewed Jul 7, 2023

View reviewed changes

auduchinok reviewed Jul 11, 2023

View reviewed changes

auduchinok reviewed Jul 13, 2023

View reviewed changes

dawedawe force-pushed the ValueNotContainedMutabilityLiteralConstantValuesDiffer branch from 1f661f6 to f63d3ff Compare July 26, 2023 15:22

dawedawe changed the base branch from net232 to net233 July 26, 2023 15:22

nojaf and others added 24 commits August 22, 2023 11:21

Quickfix for difference in mutability in signature file.

b5f622a

Update error message in the ID.

676ef15

Start working on quickfix for ValueNotContainedMutabilityLiteralConst…

3e7d62e

…antValuesDiffer

rename

b94f311

update signature, not impl

refactor

243dc20

update type in signature file if needed

9c36e72

add test case with inline comments

d6b0b5e

rename error to LiteralConstantValuesDifferError, no need to use the …

3139293

…exact same name as in FSComp.txt

format changes

1cd7b7c

check if type of impl can be resolved in sig before making fix available

215b53a

- handle Literals with other Literals as their expression

38dd977

- handle symbol availability from opened modules

add test for complex expressions fom lang preview

c2461c2

improve naming of tests

12c4470

add overload of ResolveWithFcs that takes a context node

eb494d3

add test with parens in the pattern

cf75f65

- improve naming

3b96855

- refactor to use ResolveWithFcs() - support patterns with parens

Add validation of binary expressions

119aca3

the FCS error can also mean that the impl side lacks the Literal attr…

e8d43c8

…ibute

- use IgnoreInnerParens()

4cf1127

- pass opName to ResolveWithFcs - pass true as resolveExpr to ResolveWithFcs - pass true as qualified to ResolveWithFcs

tighten match of expression

fc3f30a

rename "UpdateLiteralConstantFix" to "UpdateLiteralConstantInSignatur…

9b12f36

…eFix" rename folder of test files

rename error from "LiteralConstantValuesDiffer" to "LiteralConstantVa…

71d8675

…luesDifferInSignature"

for the time being, don't offer the fix for measure constants

18b5e8c

Support validation checks for Constants with units of measure types

35a4825

dawedawe force-pushed the ValueNotContainedMutabilityLiteralConstantValuesDiffer branch from 67e9d3e to 35a4825 Compare August 22, 2023 09:53

Don't update the return type for UoM as FSharpType.Format is currentl…

884e2a6

…y broken for UoM

DedSec256 mentioned this pull request Aug 24, 2023

Add ExtendedData to FSharpDiagnostic dotnet/fsharp#15840

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

QuickFix for ValueNotContainedMutabilityLiteralConstantValuesDiffer #546

QuickFix for ValueNotContainedMutabilityLiteralConstantValuesDiffer #546

dawedawe commented Jul 7, 2023 •

edited

auduchinok Jul 7, 2023

auduchinok Jul 7, 2023

auduchinok Jul 7, 2023

auduchinok Jul 7, 2023

dawedawe Jul 10, 2023

auduchinok left a comment

auduchinok Jul 11, 2023

dawedawe Jul 13, 2023

dawedawe Jul 14, 2023

auduchinok Jul 11, 2023

dawedawe Jul 13, 2023

auduchinok Jul 13, 2023 •

edited

auduchinok left a comment

auduchinok Jul 13, 2023

auduchinok Jul 13, 2023

auduchinok Jul 13, 2023

auduchinok Jul 13, 2023

dawedawe Jul 13, 2023

auduchinok Jul 13, 2023

auduchinok Jul 13, 2023

dawedawe Jul 13, 2023

auduchinok Jul 13, 2023

auduchinok Jul 13, 2023

dawedawe commented Jul 27, 2023

dawedawe commented Aug 23, 2023

		match bindingSignature.HeadPattern with
		\| :? IReferencePat as sigRefPat when

QuickFix for ValueNotContainedMutabilityLiteralConstantValuesDiffer #546

Are you sure you want to change the base?

QuickFix for ValueNotContainedMutabilityLiteralConstantValuesDiffer #546

Conversation

dawedawe commented Jul 7, 2023 • 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

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

auduchinok Jul 13, 2023 • 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

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

dawedawe commented Jul 27, 2023

dawedawe commented Aug 23, 2023

dawedawe commented Jul 7, 2023 •

edited

auduchinok Jul 13, 2023 •

edited