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

Add symbol creation for an operator with a constraint #6307

Merged
merged 1 commit into from
Mar 9, 2019
Merged

Add symbol creation for an operator with a constraint #6307

merged 1 commit into from
Mar 9, 2019

Conversation

7sharp9
Copy link
Contributor

@7sharp9 7sharp9 commented Mar 6, 2019

This addresses a situaltion where if you request a tooltip for an operator with a constraint no symbol is returned due to:

Item.ImplicitOp(, { contents = Some(TraitConstraintSln.FSMethSln(, vref, _)) })
Being turned into a vanilla FSharpSymbol which has no real information about the symbol that the IDE can use.

An corresponding issue was logged for ionide here: ionide/ionide-vscode-fsharp#1025

Corresponding PR at FCS: fsharp/fsharp-compiler-docs#896

This addresses a situaltion where if you request a tooltip for an operator with a constraint no symbol is returned due to:

Item.ImplicitOp(_, { contents = Some(TraitConstraintSln.FSMethSln(_, vref, _)) })
Being turned into a vanilla FSharpSymbol which has no real information about the symbol that the IDE can use.

An corresponding issue was logged for ionide here: ionide/ionide-vscode-fsharp#1025

Corresponding PR at FCS: fsharp/fsharp-compiler-docs#896
@7sharp9
Copy link
Contributor Author

7sharp9 commented Mar 6, 2019

As for recreating a tiny-tot issue for this, I did try but it was really difficult to replicate the issue, as described here: ionide/ionide-vscode-fsharp#1025

To replicate the issue hover the <*> or <!> operators in this repo:
https://github.com/jet/falanx/blob/master/src/Falanx.Proto.Core/JsonCodec/Codec.fs#L521

And a | Item.ImplicitOp(_, { contents = Some(TraitConstraintSln.FSMethSln(_, vref, _)) }) is returned resulting in a base symbol FSharpSymbol being created rather than a FSharpMemberOrFunctionOrValue type.

Copy link
Contributor

@cartermp cartermp left a comment

Choose a reason for hiding this comment

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

@KevinRansom
Copy link
Member

@7sharp9 thanks for this

Kevin

@KevinRansom KevinRansom merged commit cfc9f37 into dotnet:master Mar 9, 2019
@@ -290,6 +290,9 @@ type FSharpSymbol(cenv: SymbolEnv, item: (unit -> Item), access: (FSharpSymbol -
| Item.ArgName(id, ty, _) ->
FSharpParameter(cenv, ty, {Attribs=[]; Name=Some id}, Some id.idRange, isParamArrayArg=false, isInArg=false, isOutArg=false, isOptionalArg=false) :> _

| Item.ImplicitOp(_, { contents = Some(TraitConstraintSln.FSMethSln(_, vref, _)) }) ->
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a good fix, though we should also cover the case where the solution is a .NET Method or an F# record field (the other possibliies for TestConstraintSln)

@7sharp9 Would you be able to submit a new PR for those 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.

@dsyme , yeah I can add the others too.
Testing these is really tough though as Ive only been able to replicate the issue within Falanx referencing a Fleece operator, which references FSharpPlus operator. i.e.No simple minimal repo. There may be other symbols that we could surface too...

Copy link
Contributor Author

@7sharp9 7sharp9 Mar 10, 2019

Choose a reason for hiding this comment

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

Incidentally @dsyme Do you know why ItemWithInst.TyparInst is always empty for custom operators?

https://github.com/Microsoft/visualfsharp/blob/a26d32a86c88f4ae61a77bfe5c764cb0ea8cec8b/src/fsharp/NameResolution.fsi#L133

Copy link
Contributor

Choose a reason for hiding this comment

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

Custom operators (in computation expressions) or implicit operators?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Implicit operators, they seem to match always get created with ItemWithNoInst

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.

5 participants