Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

QuickFix for ValueNotContainedMutabilityLiteralConstantValuesDiffer #546

Draft
wants to merge 25 commits into
base: net233
Choose a base branch
from

Conversation

dawedawe
Copy link
Contributor

@dawedawe dawedawe commented Jul 7, 2023

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

Comment on lines 64 to 68
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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

let typeUsage = factory.CreateTypeUsage(returnTypeString, TypeUsageContext.TopLevel)
sigRefPat.Binding.ReturnTypeInfo.SetReturnType(typeUsage) |> ignore

sigRefPat.Binding.SetExpression(error.Pat.Binding.Expression.Copy()) |> ignore
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

[<Literal>]
let MyLiteral = 1

[<Literal>]
let a = MyLiteral

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please also consider more complex literal expressions:

[<Literal>]
let a = 1 + 1

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

@auduchinok auduchinok left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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. 🙂

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll have a look at that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines 29 to 30
match bindingSignature.HeadPattern with
| :? IReferencePat as sigRefPat when
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it work with parens?

[<Literal>]
let (a) = 1

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

@auduchinok auduchinok Jul 13, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

@auduchinok auduchinok left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

let mutable sigRefPat = null

let rec isImplExprValidInSig (implExpression: IFSharpExpression) =
match implExpression with
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use implExpression.IgnoreInnerParens()

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please pass something like UpdateLiteralConstantFix.IsAvailable as the opName.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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. 🙂

open JetBrains.ReSharper.Psi.Tree
open JetBrains.ReSharper.Resources.Shell

type UpdateLiteralConstantFix(error: LiteralConstantValuesDifferError) =
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 dawedawe force-pushed the ValueNotContainedMutabilityLiteralConstantValuesDiffer branch from 1f661f6 to f63d3ff Compare July 26, 2023 15:22
@dawedawe dawedawe changed the base branch from net232 to net233 July 26, 2023 15:22
@dawedawe
Copy link
Contributor Author

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.

nojaf and others added 24 commits August 22, 2023 11:21
update signature, not impl
- handle symbol availability from opened modules
- refactor to use ResolveWithFcs()
- support patterns with parens
- pass opName to ResolveWithFcs
- pass true as resolveExpr to ResolveWithFcs
- pass true as qualified to ResolveWithFcs
@dawedawe dawedawe force-pushed the ValueNotContainedMutabilityLiteralConstantValuesDiffer branch from 67e9d3e to 35a4825 Compare August 22, 2023 09:53
@dawedawe
Copy link
Contributor Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants