fix isClassNodeIdentifier in hls-class-plugin #4020
Conversation
soulomoon
changed the title
| @@ -198,8 +198,9 @@ 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 | |||
| isClassNodeIdentifier :: Identifier -> IdentifierDetails a -> Bool | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
There are many holes in this plugin.
- bug in isClassNodeIdentifier that do not classify idenfitifer correctly.
- 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 :: IdentifierDetails a -> Bool | ||
| isClassNodeIdentifier ident = (isNothing . identType) ident && Use `Set.member` identInfo ident | ||
| isClassNodeIdentifier :: Identifier -> IdentifierDetails a -> Bool | ||
| isClassNodeIdentifier (Right i) ident | 'C':':':_ <- unpackFS $ occNameFS $ occName i = (isNothing . identType) ident && Use `Set.member` identInfo ident |
There was a problem hiding this comment.
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..
There was a problem hiding this comment.
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
There was a problem hiding this comment.
This is a little obscure - worth a comment. Perhaps @wz1000 can verify if this is indeed the best way.
soulomoon
requested review from
jhrcek and
July541
and removed request for
jhrcek
|
Thank you for your reviews @jhrcek, I have made some responses. |
This comment has been minimized.
This comment has been minimized.
|
Ooops, it turns out I was testing the local builds incorrectly. I apologize for the confabulated attempts at explanations why this PR doesn't work - this PR actually DOES prevent the error from being thrown in UI. Though I must admin I don't understand HOW the fix makes the error go away 😄 |
|
No worries @jhrcek thank you for testing this out. This Pr fix this situation. Hence, the error goes away. (Data constructor for a type class is generated by ghc, it is the way ghc handle type classes) |
|
Can we add an example from the ticket as a test to ensure this keeps working? |
See #4020 (comment) |
…inittcwithgbl-failed
…inittcwithgbl-failed
…inittcwithgbl-failed
…inittcwithgbl-failed
|
I managed to come up with a test that focus on the |
…ss-causes-internal-error-inittcwithgbl-failed' into 3942-deriveanyclass-causes-internal-error-inittcwithgbl-failed
Co-authored-by: Michael Peyton Jones <me@michaelpj.com>
Partially fix #3942, by handling isClassNodeIdentifier correctly.