hls-class-plugin: Only create placeholders for unimplemented methods #2956
Conversation
akshaymankar
force-pushed
the
class-plugin-detect-unimplemented
branch
from
e7949dd
to
d15cdde
Compare
akshaymankar
force-pushed
the
class-plugin-detect-unimplemented
branch
from
d15cdde
to
941b557
Compare
|
cc @July541 |
| (HAR {hieAst = hf}, pmap) <- MaybeT . runAction "classplugin" state $ useWithStale GetHieAst docPath | ||
| pure | ||
| $ concat | ||
| $ pointCommand hf (fromJust (fromCurrentRange pmap range) ^. J.start & J.character -~ 1) |
There was a problem hiding this comment.
It would be great if we could reduce the partial function usage.
There was a problem hiding this comment.
especially since you're already returning maybes and lists, so you have multiple ways of dealing with missing things!
There was a problem hiding this comment.
Um, I am not sure which function is partial here, Can you please help me with that?
There was a problem hiding this comment.
fromJust will crash the program if the argument is Nothing!
There was a problem hiding this comment.
Oh right, i copy-pasted it from above and didn't even see 🙈
|
|
||
| #if MIN_VERSION_ghc(9,2,0) | ||
| import GHC.Hs (AnnsModule(AnnsModule)) | ||
| import GHC.Parser.Annotation | ||
| #endif | ||
|
|
||
| descriptor :: PluginId -> PluginDescriptor IdeState | ||
| descriptor plId = (defaultPluginDescriptor plId) | ||
| newtype Log = Log T.Text deriving Show |
There was a problem hiding this comment.
I think, it would be a good idea to change this to more explicit constructors instead of a generic text type. The idea being, that the structured logging can have added information later, but if it is unstructured using text, we lose this structured info. In particular, I propose to change it to the following Log type:
| newtype Log = Log T.Text deriving Show | |
| data Log | |
| = LogImplementedMethods [T.Text] | |
| deriving Show |
and changing the Pretty method to prepend the message we currently have in the log message further down.
There was a problem hiding this comment.
I also added name of the class to the log.
| $ pointCommand hf (fromJust (fromCurrentRange pmap range) ^. J.start & J.character -~ 1) | ||
| $ map (T.pack . getOccString) . rights . findInstanceValBindIdentifiers | ||
|
|
||
| findInstanceValBindIdentifiers :: HieAST a -> [Identifier] |
There was a problem hiding this comment.
This could do with some haddock and/or explanation. what is it doing and why?
There was a problem hiding this comment.
I added an explanation, please let me know if it makes sense.
There was a problem hiding this comment.
Why can't we just transitively look for all the things that are InstanceValBinds underneath, rather than having to specifically say which things we'll recurse through? I'd have thought that the only InstanceValBinds in a instance declaration would be precisely the already-implemented methods?
There was a problem hiding this comment.
Hmm, yeah makes the code much simpler:
findInstanceValBindIdentifiers :: HieAST a -> [Identifier]
findInstanceValBindIdentifiers ast =
let valBindIds = Map.keys
. Map.filter (any isInstanceValBind . identInfo)
$ getNodeIds ast
in valBindIds <> concatMap findInstanceValBindIdentifiers (nodeChildren ast)I guess this will perform worse as this is now traversing more of the AST, but maybe this is not as bad?
There was a problem hiding this comment.
Actually, this also makes the code compatible with older GHCs, so I am going to commit it.
akshaymankar
force-pushed
the
class-plugin-detect-unimplemented
branch
2 times, most recently
from
f63f18f
to
79d0f7b
Compare
akshaymankar
changed the title
akshaymankar
force-pushed
the
class-plugin-detect-unimplemented
branch
from
79d0f7b
to
da0d388
Compare
| $ nodeIds | ||
| where | ||
| nodeIds = getNodeIds ast | ||
| hasEvidenceBind = not . Map.null . Map.filter (any isEvidenceBind . identInfo) |
There was a problem hiding this comment.
| hasEvidenceBind = not . Map.null . Map.filter (any isEvidenceBind . identInfo) | |
| hasEvidenceBind m = any isEvidenceBind $ concatMap (identInfo) $ Map.elems m |
There was a problem hiding this comment.
I couldn't use concatMap as identiInfo returns a Set, so I used mconcat $ map identInfo $ ...
| $ pointCommand hf (fromJust (fromCurrentRange pmap range) ^. J.start & J.character -~ 1) | ||
| $ map (T.pack . getOccString) . rights . findInstanceValBindIdentifiers | ||
|
|
||
| findInstanceValBindIdentifiers :: HieAST a -> [Identifier] |
There was a problem hiding this comment.
Why can't we just transitively look for all the things that are InstanceValBinds underneath, rather than having to specifically say which things we'll recurse through? I'd have thought that the only InstanceValBinds in a instance declaration would be precisely the already-implemented methods?
| @@ -234,18 +253,55 @@ codeAction state plId (CodeActionParams _ _ docId _ context) = liftIO $ fmap (fr | |||
| _ -> panic "Ide.Plugin.Class.findClassFromIdentifier" | |||
| findClassFromIdentifier _ (Left _) = panic "Ide.Plugin.Class.findClassIdentifier" | |||
|
|
|||
| findImplementedMethods :: NormalizedFilePath -> Range -> MaybeT IO [T.Text] | |||
| findImplementedMethods docPath range = do | |||
| (HAR {hieAst = hf}, pmap) <- MaybeT . runAction "classplugin" state $ useWithStale GetHieAst docPath | |||
There was a problem hiding this comment.
Both this and findClassIdentifier run an action to get the HIE AST. I'm not 100% sure, but I suspect it may be more efficient to only do that once. Perhaps pull the action out of the two functions and just pass in the HIE ast to them?
There was a problem hiding this comment.
Also, they both run a pointCommand over the same range. Maybe not worth the refactoring, but in principle you could just run one that returns both the things we're interested in.
akshaymankar
force-pushed
the
class-plugin-detect-unimplemented
branch
2 times, most recently
from
921fb0d
to
512ad1a
Compare
akshaymankar
force-pushed
the
class-plugin-detect-unimplemented
branch
from
512ad1a
to
3768251
Compare
…askell#2956) * hls-class-plugin: Only create placeholders for unimplemented methods * hls-class-plugin: Add logs
Before this change, the
hls-class-pluginwould offer to create placeholders for methods which were already implemented. This change walks through the AST of the instance and figure out which methods exist and only offer to implement those which don't already exist.