Skip to content

fix isClassNodeIdentifier in hls-class-plugin #4020

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

8 changes: 5 additions & 3 deletions plugins/hls-class-plugin/src/Ide/Plugin/Class/CodeAction.hs
Original file line number Diff line number Diff line change
@@ -159,7 +159,7 @@ codeAction recorder state plId (CodeActionParams _ _ docId _ context) = do
$ listToMaybe
$ mapMaybe listToMaybe
$ pointCommand hf instancePosition
( (Map.keys . Map.filter isClassNodeIdentifier . getNodeIds)
( (Map.keys . Map.filterWithKey isClassNodeIdentifier . getNodeIds)
<=< nodeChildren
)

@@ -198,8 +198,10 @@ codeAction recorder state plId (CodeActionParams _ _ docId _ context) = do
_ -> fail "Ide.Plugin.Class.findClassFromIdentifier"
findClassFromIdentifier _ (Left _) = throwError (PluginInternalError "Ide.Plugin.Class.findClassIdentifier")

isClassNodeIdentifier :: IdentifierDetails a -> Bool
isClassNodeIdentifier ident = (isNothing . identType) ident && Use `Set.member` identInfo ident
-- see https://hackage.haskell.org/package/ghc-9.8.1/docs/src/GHC.Types.Name.Occurrence.html#mkClassDataConOcc
isClassNodeIdentifier :: Identifier -> IdentifierDetails a -> Bool
Copy link
Collaborator

@jhrcek jhrcek Jan 27, 2024

Choose a reason for hiding this comment

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

Were you able to reproduce the bug? It would be great (and shouldn't be that hard) to add a test that reproduces the original issue and confirms this fix works.

Copy link
Collaborator Author

@soulomoon soulomoon Jan 27, 2024

Choose a reason for hiding this comment

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

There are many holes in this plugin.

  1. bug in isClassNodeIdentifier that do not classify idenfitifer correctly.
  2. also there is another bug, we should not invoke such code action in deriving expression, which require us to back trace to its parents to see if it is derving expression.

I should mark this pull request as a partial fix, which solve 1.
Currently, I do not see a way to trigger the bug in 1 without 2.

isClassNodeIdentifier (Right i) ident | 'C':':':_ <- unpackFS $ occNameFS $ occName i = (isNothing . identType) ident && Use `Set.member` identInfo ident
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 could use something like: https://hackage.haskell.org/package/ghc-9.8.1/docs/src/GHC.Types.Name.Occurrence.html#isTcOcc on the occName to check if it's a name of a type class, rather than all the unpacking..

Copy link
Collaborator Author

@soulomoon soulomoon Jan 27, 2024

Choose a reason for hiding this comment

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

I do not see there is a isTcOcc equivalent function of checking data constructor for class. 🤔。eventhough DataName appears so, but it should be for all data constructors and not only data constructors for class

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is a little obscure - worth a comment. Perhaps @wz1000 can verify if this is indeed the best way.

Copy link
Collaborator

Choose a reason for hiding this comment

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

nag @wz1000

isClassNodeIdentifier _ _ = False

isClassMethodWarning :: T.Text -> Bool
isClassMethodWarning = T.isPrefixOf "• No explicit implementation for"
12 changes: 12 additions & 0 deletions plugins/hls-class-plugin/test/Main.hs
Original file line number Diff line number Diff line change
@@ -9,6 +9,7 @@ module Main
( main
) where

import Control.Exception (catch)
import Control.Lens (Prism', prism', view, (^.),
(^..), (^?))
import Control.Monad (void)
@@ -120,6 +121,17 @@ codeLensTests = testGroup
doc <- openDoc "TH.hs" "haskell"
lens <- getAndResolveCodeLenses doc
liftIO $ length lens @?= 0
, testCase "Do not construct error action!, Ticket3942one" $ do
runSessionWithServer def classPlugin testDataDir $ do
doc <- openDoc "Ticket3942one.hs" "haskell"
_ <- waitForDiagnosticsFromSource doc (T.unpack sourceTypecheck)
lens <- getAllCodeActions doc
-- should switch to `liftIO $ length lens @?= 2, when Ticket3942 is entirely fixed`
-- current fix is just to make sure the code does not throw an exception that would mess up
-- the client UI.
liftIO $ length lens > 0 @?= True
`catch` \(e :: SessionException) -> do
liftIO $ assertFailure $ "classPluginTestError: "++ show e
, goldenCodeLens "Apply code lens" "CodeLensSimple" 1
, goldenCodeLens "Apply code lens for local class" "LocalClassDefine" 0
, goldenCodeLens "Apply code lens on the same line" "Inline" 0
13 changes: 13 additions & 0 deletions plugins/hls-class-plugin/test/testdata/Ticket3942one.hs
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
{-# LANGUAGE DeriveAnyClass #-}

module Ticket3942one where

class C a where
foo :: a -> Int

newtype Foo = MkFoo Int deriving (C)
instance Show Foo where


main :: IO ()
main = return ()