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.

Sign up for GitHub

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

Jump to bottom

fix isClassNodeIdentifier in hls-class-plugin #4020

Merged
merged 10 commits into from
Feb 7, 2024
Merged

fix isClassNodeIdentifier in hls-class-plugin #4020

Changes from 1 commit
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
0c87468
fix isClassNodeIdentifier
soulomoon Jan 27, 2024
af8e290
Merge branch 'master' into 3942-deriveanyclass-causes-internal-error-…
soulomoon Jan 29, 2024
19b3791
Merge branch 'master' into 3942-deriveanyclass-causes-internal-error-…
soulomoon Feb 3, 2024
7a7e8b5
Merge branch 'master' into 3942-deriveanyclass-causes-internal-error-…
soulomoon Feb 7, 2024
919d78a
add comment
soulomoon Feb 7, 2024
7ab3646
add test
soulomoon Feb 7, 2024
78d8cc7
Merge branch 'master' into 3942-deriveanyclass-causes-internal-error-…
soulomoon Feb 7, 2024
fc37c93
cleanup
soulomoon Feb 7, 2024
b7e1f17
Merge remote-tracking branch 'refs/remotes/upstream/3942-deriveanycla…
soulomoon Feb 7, 2024
a252542
Update plugins/hls-class-plugin/test/Main.hs
soulomoon Feb 7, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions plugins/hls-class-plugin/src/Ide/Plugin/Class/CodeAction.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
)

Expand Down Expand Up @@ -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
Copy link
Collaborator

@jhrcek jhrcek Jan 27, 2024 •
edited
Loading

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 •
edited
Loading

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 •
edited
Loading

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"
Expand Down
Loading