class-plugin: Add code action for implementing instance method when default implementation doesn't work #3267
Conversation
akshaymankar
force-pushed
the
class-implement-default-methods
branch
2 times, most recently
from
b755fd4 to
e5a386d
Compare
|
Sorry for the delay, I'll review this on weekend. |
| @@ -207,6 +213,9 @@ isClassNodeIdentifier ident = (isNothing . identType) ident && Use `Set.member` | |||
| isClassMethodWarning :: T.Text -> Bool | |||
| isClassMethodWarning = T.isPrefixOf "• No explicit implementation for" | |||
|
|
|||
| isInstanceMissingError :: T.Text -> Bool | |||
| isInstanceMissingError = T.isPrefixOf "• No instance for" | |||
There was a problem hiding this comment.
What if this error comes from something not in an instance definition?
If I guess correctly, this will give error responses, which is not so desirable.
I think it's probably better to have a more specific way to detect this "invalid default impl" case.
There was a problem hiding this comment.
Hmm, yes this produces logs like Warning | class: InternalError: No Identifier found. I am not sure what is a better way to detect this error, here are two errors:
• No instance for (Test1 X)
arising from a use of ‘DefaultImplementation.$dmf’
• In the expression: DefaultImplementation.$dmf @(X)
In an equation for ‘f’: f = DefaultImplementation.$dmf @(X)
In the instance declaration for ‘Test X’
• No instance for (Show a) arising from a use of ‘show’
Possible fix:
add (Show a) to the context of
the type signature for:
foo :: forall a. a -> String
• In the expression: show
In an equation for ‘foo’: foo = show
generated from file:
{-# LANGUAGE DefaultSignatures #-}
module DefaultImplementation where
class Test1 a where
class Test a where
f :: a -> a
default f :: Test1 a => a -> a
f = id
data X = X | Y
foo :: a -> String
foo = show
instance Test X where
Do you think trying to see if there is no "Possible fix" would be better? Or should the plugin just start expecting this other case and not throw errors when it doesn't find any class identifier?
There was a problem hiding this comment.
How about looking for the "In instance" part? That sounds more promising. And surely it'd be better if it doesn't report HLS errors for non-default instance resolution GHC errors.
There was a problem hiding this comment.
Makes sense, I added a check for both now.
There was a problem hiding this comment.
BTW, how does this work for something like
data X = X | Y
class Test a where
f :: a -> String
instance Test X where
f = show?
| let groups = | ||
| -- If the mininal def contains nothing, perhaps there are | ||
| -- default implementations of methods, so the LSP offers to | ||
| -- add stub implementations of all methods. |
There was a problem hiding this comment.
As we have cls, we don't need to guess whether there are default method impls or not. We can use classOpItems to get the information.
There was a problem hiding this comment.
Thanks! I was not aware of classOpItems 👍
akshaymankar
force-pushed
the
class-implement-default-methods
branch
2 times, most recently
from
4447a49 to
7046f69
Compare
…efault implementation doesn't work
akshaymankar
force-pushed
the
class-implement-default-methods
branch
from
7046f69 to
f1e6bdb
Compare
|
@akshaymankar Anything blocking this? If not, I am going to give it a quick review and then merge it ;D |
I don't think so. Ship it if it looks good! |
|
|
||
| classOpItemsGroups :: Range -> [InstanceBindTypeSig] -> Class -> [[(T.Text, T.Text)]] | ||
| classOpItemsGroups range sigs cls = | ||
| concatMap (mkGroups range sigs . getName . fst) $ classOpItems cls |
There was a problem hiding this comment.
These are not just ones with default impl, but all methods, aren't they?
Half of what #3135 proposes