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 local definitions to outline #4312

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

sgillespie
Copy link
Contributor

Addresses #3628. Adds all local symbols (functions/vars) as children to function document symbols.

There are a few potential issues:

  1. The position/range points to the toplevel function
  2. getFuzzyScope might be abused here (should I create another function like getLocalScope?)

Thoughts?

@sgillespie sgillespie requested a review from wz1000 as a code owner June 11, 2024 14:35
@sgillespie
Copy link
Contributor Author

sgillespie commented Jun 11, 2024

This is what the outline looks like in emacs (via imenu-list)

ss-20240611-103517

For a bit of extra context, here is one of the definitions:

f :: a -> a
f x = g x
  where
    g = id
    h = id

getRefMap :: HieAstResult -> RefMap Type
getRefMap HAR{refMap=refMap, hieKind=hieKind} =
case hieKind of
HieFromDisk _ -> mempty
Copy link
Collaborator

Choose a reason for hiding this comment

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

I have a question, why we use mempty for HieFromDisk here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is mostly because I needed a RefMap Type, and I didn't know how to get there from RefMap TypeIndex. If this is important, I can keep hacking.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe you can find some inspiration for how to recover full types here?

HieFresh -> printOutputable t
HieFromDisk full_file -> printOutputable $ hieTypeToIface $ recoverFullType t (hie_types full_file)

@@ -41,11 +45,13 @@ moduleOutline ideState _ DocumentSymbolParams{ _textDocument = TextDocumentIdent
= liftIO $ case uriToFilePath uri of
Just (toNormalizedFilePath' -> fp) -> do
mb_decls <- fmap fst <$> runAction "Outline" ideState (useWithStale GetParsedModule fp)
mb_hieAst <- fmap fst <$> runAction "Outline" ideState (useWithStale GetHieAst fp)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmmm, these are both ignoring the position mapping, which is probably risky? It means we might create document symbols with out-of-date locations.

@@ -282,4 +305,11 @@ hsConDeclsBinders cons
-> [LFieldOcc GhcPs]
get_flds flds = concatMap (cd_fld_names . unLoc) (unLoc flds)


getLocalBindings :: RefMap Type -> LHsDecl GhcPs -> [Name]
Copy link
Collaborator

Choose a reason for hiding this comment

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

This could do with some explanation: I'm not sure reading this why this is the right way to get the local bindings.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What is confusing? Is it the use of getFuzzyScope or is it something else?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess I'm just rather unsure why this is the right thing to do? Why is it okay to use getFuzzyScope rather than getLocalScope? What's going on with position mapping?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

According to my testing, getLocalScope only returns symbols at the current start point (the start of the given function), which does not appear to have the locals in scope. getFuzzyScope returns all symbols that have intersecting scopes. This makes sense to me when I look at the docs for IntervalMap.FingerTree's dominator (getLocalScope) and intersections (getFuzzyScope):

dominators:

O(k log (n/k)). All intervals that contain the given interval, in lexicographical order.

intersections

O(k log (n/k)). All intervals that intersect with the given interval, in lexicographical order.

It's clear that there at least needs a comment there, but maybe it would be better to create a new function with a clearer name?

Copy link
Collaborator

Choose a reason for hiding this comment

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

According to my testing, getLocalScope only returns symbols at the current start point (the start of the given function), which does not appear to have the locals in scope.

I feel confused about what getLocalScope is for then, if not this? Are we using it wrong?

getLocalBindings refmap (L (locA -> (RealSrcSpan l _)) _) =
nubOrdOn getOccFS . filter isVarName . map fst $ locals
where
locals = getFuzzyScope (bindings refmap) start end
Copy link
Collaborator

Choose a reason for hiding this comment

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

This doesn't recurse, right? So presumably this won't handle something like:

f = ...
  where
     g = ...
       where
          h = ...

?

Worth adding as a test case even if we don't support it, but also maybe we just can support it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It actually does recurse, I've just pushed an update that adds this and the other test case below

@michaelpj
Copy link
Collaborator

A test case I'd like to see

f 1 = g
  where g = 2
f _ = g 
  where g = 3

Copy link
Collaborator

@michaelpj michaelpj left a comment

Choose a reason for hiding this comment

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

Tests are helpful! I think the results look a bit off?

(R 0 0 0 34)
[ docSymbol "x" SymbolKind_Function (R 0 0 0 34)
, docSymbol "g" SymbolKind_Function (R 0 0 0 34)
, docSymbol "h" SymbolKind_Function (R 0 0 0 34)
Copy link
Collaborator

Choose a reason for hiding this comment

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

These locations are wrong? They all have the range of the original binding?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also, shouldn't h be a child of g, not of a?

"f"
SymbolKind_Function
(R 0 0 1 19)
[docSymbol "g" SymbolKind_Function (R 0 0 1 19)]
Copy link
Collaborator

Choose a reason for hiding this comment

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

This seems wrong, there should be two gs?

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.

4 participants