Proof of concept fix to get a tooltip when inside a ActivePattern case #503
Conversation
| // todo: fix getting qualifiers | ||
| let tokenNames = [token.Name] | ||
| let treeEndOffset = | ||
| if (isNull token.Parent) then token.GetTreeEndOffset() else |
There was a problem hiding this comment.
Didn't the first Parent already have some active pattern-like type?
Maybe worth checking on that as well.
| let tokenNames = [token.Name] | ||
| let treeEndOffset = | ||
| if (isNull token.Parent) then token.GetTreeEndOffset() else | ||
| if (isNull token.Parent.Parent) then token.GetTreeEndOffset() else |
There was a problem hiding this comment.
Going up the tree with Parent properties without checking the node types and children roles is very dangerous because it may work in too many places where you may not expect it. Unless you want this check to pass for every identifier that has a parent (and the most of them do indeed have one), you should check what parents are and, where applicable, from which of the children you go up. This is why the generated navigators should be used 95% of the time.
If you're interested in active pattern nodes, the please check that the parent node is an active pattern one. Please check the parsed tree.
There was a problem hiding this comment.
Please note that IActivePatternId is also an IFSharpIdentifier, as well as ITypeParameterId, so it may worth checking if detecting and passing them instead of the initial inner identifiers could fix it too.
| let barTreeNode = token.Parent.Parent.Children() |> Seq.tryFindBack(isBar) | ||
| match barTreeNode with | ||
| | Some n -> n.GetTreeEndOffset() |
There was a problem hiding this comment.
We're lucky that there are currently no other rules in the grammar that would match it, but if we change a structure of anything that contains | then it may suddenly match this check. Consider:
type E =
| A = 1type U =
| U of i: intmatch () with
| x -> ()match () with
| a | b -> ()I can imagine that we could want to remove the field list node in the second case in the future (i.e. move the field nodes to the case node), and that would suddenly break the tooltips for i because of this check.
| | Some n -> n.GetTreeEndOffset() | ||
| | None -> token.GetTreeEndOffset() | ||
| match token.Parent with | ||
| | :? ITopActivePatternCaseDeclaration as iTop when isNotNull iTop -> |
There was a problem hiding this comment.
Why do you check isNotNull iTop after the successful 'instance of' check?
There was a problem hiding this comment.
This check only works for top-level active patterns, not for local ones:
let (|A|B|) x = ()
do
let (|C|D|) x = ()
()Please try to use base/more common interfaces where possible.
- remove overzealous null check
| | :? IFSharpIdentifier as node when node.GetSourceFile() = sourceFile -> Some node | ||
| | :? IFSharpIdentifier as node when node.GetSourceFile() = sourceFile -> | ||
| let activePatternCaseName = ActivePatternCaseNameNavigator.GetByIdentifier(node) | ||
| if isNotNull activePatternCaseName then |
There was a problem hiding this comment.
If I remember correctly, the Navigators are allowed to take null as input.
So the null check should only happen for activePatternId.
- adjust DoTest() to be able to work with FSharp QuickDoc Html
| @@ -0,0 +1,3 @@ | |||
| <html> | |||
| <body> | |||
| <div style="float:right" align="right"><a href="http://goto/">go to</a></div><div class="headers"><dl class="headers"><dt>public static method FSharpChoice<Unit,Unit> <b> |Even|Odd|</b></dt></dl></div>Some Doc on ActivePattern</body></html> No newline at end of file | |||
There was a problem hiding this comment.
Hm, these 'public static method' tooltips shouldn't be visible in F#. I think it's OK to merge it and to fix it separately.
|
@dawedawe Thanks a lot! 🙂 |
3 participants
This is a proof-of-concept fix for the observed behaviour here
If the general approach is acceptable we would try to fix this in a somewhat cleaner way in FSharpQuickDocProvider.tryFindToken().
The results are:
