haskell-language-server icon indicating copy to clipboard operation
haskell-language-server copied to clipboard

Code action to add deriving instance

Open guibou opened this issue 2 years ago • 4 comments

Is your feature request related to a problem? Please describe.

I'd like a way to add an instances to the deriving clause of a type (and related type).

For example, I just tried to deepseq on a type in our codebase, and that type had no NFData. It took me something like 2 hours (and would have be way more without HLS) to just jump to definition of type, add the instance, jump to the definition of sub types, add the instances, add import, add LANGUAGE for DerivingGeneric, jump to another type. Rinse and repeat.

Describe the solution you'd like

I'd like, in case of Could not deduce (InstanceName AType) error, to have a code action Add instance InstanceName to AType.

As a first step, it would jump to the type and add the instance to the deriving clause. As a second step, it would be great if it could recursively add the instance to all the related types (i.e. loop until there is no more Could not deduce ... error.

Some questions:

  • In case of DerivingStrategies, In which clause should the instance go?
  • What if we need to add some LANGUAGE pragma?
  • What if we need to add some import
  • Can we do the recursive addition in one step, or should we iterate until there is no more error message? What about performances here?

Describe alternatives you've considered

Not adding this plugin?

Additional context

guibou avatar Feb 18 '22 08:02 guibou

I'm wondering if that's something https://hackage.haskell.org/package/retrie-1.2.0.0 could do for us.

If you have ideas about how to solve the recursion in a robust (and fast) way, I'll be happy to try an implementation. But for now I'm not sure about the feasibility of such a code action.

guibou avatar Feb 18 '22 08:02 guibou

I don't think that retrie would be a good fit for this, since it doesn't interact with the typechecker nor the diagnostics.

For HLS, I can see how the code action for adding a deriving instance would work, but I don't think it can do the recursive additions.

This problem with recursively propagating a fix appears in other use cases too:

  1. Hlint, where applying a suggestion unlocks a new one
  2. Implicit parameters, where inserting an implicit parameter in a function creates a new missing implicit parameter in all its callers

Rather than making all these code actions "recursive" I would propose creating a CLI command that can apply one or more code actions recursively. It might also be possible to wrap this as an LSP command, but I'm not entirely sure.

pepeiborra avatar Feb 18 '22 10:02 pepeiborra

I also think the dumb solution is better: just do one fix. Doing many fixes is more unexpected and significantly more complicated. It's also not so bad for the user to just pump the loop of "apply code action" until things are fixed.

Add instance InstanceName to AType.

Nit: "Derive instance...", this is specifically for deriving instances. There could also be a code action that just inserts an instance declaration for you to fill in, which sounds more like "Add instance".

We probably also want variants for

  • Try and derive/add instance at the definition site
  • Try and standalone derive/add instance at the current position (in case the type is in a library)

In case of DerivingStrategies, In which clause should the instance go?

I would say we should always use DerivingStrategies, even if it wasn't used previously. Not using DerivingStrategies is IMO dangerous, especially if we're going to enable anyclass deriving.

I think we could then use an algorithm like this:

  • If the class is one of the stock-derivable classes, stock derive it
  • If not, and the type is a newtype, newtype derive it
  • If not, anyclass derive it (this may of course not work if the class isn't anyclass derivable, but we can't know that)

What if we need to add some LANGUAGE pragma? What if we need to add some import?

I think we could add them. Similarly to how we add imports for completions of out-of-scope identifiers.

michaelpj avatar Feb 18 '22 10:02 michaelpj

Thank you for your comment.

@michaelpj I agree with all of what you said, especially not doing the recursion, let's keep it simple. Actually, I realize that an automated action which just insert the deriving at the correct spot + extensions + imports + jump to next error would already have saved me minutes.

guibou avatar Feb 18 '22 17:02 guibou