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

[POC] Move plugin and command names to the typelevel #152

Merged
merged 15 commits into from Jan 7, 2016

Conversation

Projects
None yet
3 participants
@cocreature
Collaborator

cocreature commented Jan 1, 2016

This is more of a POC than finished code, but hopefully it can help to start a discussion if we want to move in that direction.

Now let me explain my motivation for doing this:

In the current state servant knows pretty much nothing about our api. All it knows is that there are two routes, but it doesn’t know which plugins exists. That means that all the nice stuff servant brings us, like auto generated bindings is pretty much useless. In particular I played around with servant-swagger which works but the documentation it generates is useless for that exact reason (pinging @tobiasgwaaler since he wanted to work on that).

With this approach we get at least the plugins and the commands correctly documented although we can and probably should take this further so the parameters a command receives are also somewhere on the typelevel so that servant-swagger can just pick them up and use them instead of saying that it needs an object.

Another cool idea would be to reuse that same type to generate the documentation and possibly also for the json stdio although I’m less sure how useful it is there. In general the servant approach seems to be to declare the API once and then generate the rest from it which I kinda like. @rvion might be interested in that since we wanted to go for code generation. Afaik there is not yet a python backend for servant but as soon as someone adds that you should be able to get bindings for the http backend for free.

Now to the downsides:

  • It adds complexity (although it was a lot of fun :)), I don’t have a problem with that but it might scare off beginners.
  • We now have three places where we have lists of plugins (PluginList and pluginList once in the documentation generator and once here). Even worse we have an additional list of command names here. Not sure how much effort we should put into fixing that until we have figured out #25 (which we can hopefully do rather soon). An upside of duplicating the command names is that at least for the servant backend (and once we use that type for the other backends also there) it allows selectively disabling certain commands. However I can’t come up with a reason why you would want to do that right now.
@alanz

This comment has been minimized.

Collaborator

alanz commented Jan 1, 2016

What worries me most about this is that we break modularity. If someone wants to add a private plugin, say to access some proprietary company resources, then they have to fork hie and add their commands in to the master type.

Conceptually it is interesting, but I wonder if the benefits it brings are worth the cost.

Not that I am against it, just very wary,

@cocreature

This comment has been minimized.

Collaborator

cocreature commented Jan 1, 2016

Thanks for noting that I somehow thought that I was working in the executable. I know moved it to the executable so it’s at least not worse than the previous state when it comes to what you have to recompile. It might make sense to create a separate package for the executable anyway since we can then more easily share configuration without running into circular dependencies.

@alanz

This comment has been minimized.

Collaborator

alanz commented Jan 1, 2016

Can we have the list of commands come out of the plugin, rather than being listed in the app?

e.g.

type PluginList = '[ 'PluginType "applyrefact" ApplyRefactListType
                           ...
@cocreature

This comment has been minimized.

Collaborator

cocreature commented Jan 1, 2016

I am pretty sure we can, I just didn’t want to put too much time into it if we decide not to go for this approach. I’ll give it a try later. If we don’t manage to remove the duplication between plugins and PluginList with enough type level hackery we could always go for template haskell. but I doubt we’ll have to resort to that.

@cocreature

This comment has been minimized.

Collaborator

cocreature commented Jan 1, 2016

I managed to get rid of the duplication so now all the plugin and command names are only declared once. It adds a bit of complexity but it wasn’t as bad as I expected it to be. Also somebody who actually knows what they’re doing will probably be able to make it a bit nicer :)

@cocreature cocreature force-pushed the cocreature:servant-fancy branch from 500c87e to fc900fe Jan 1, 2016

@cocreature

This comment has been minimized.

Collaborator

cocreature commented Jan 1, 2016

One more thing to note here, this relies on PartialTypeSignatures to avoid repeating plugin and command names. Imho this is a perfect usecase for PartialTypeSignatures since the types here are not so much intended for human reading as they are for servant, but there might be objections

@cocreature cocreature force-pushed the cocreature:servant-fancy branch from 02cbef6 to 3f67e43 Jan 4, 2016

@JPMoresmau

This comment has been minimized.

Collaborator

JPMoresmau commented Jan 4, 2016

My 2 cents: I understand the requirement, and having swagger integration will be great, but I fear the complexity for aspiring plugin writers. Heck, I have no idea what singleton types are or why I should care (-:. Maybe we should investigate how we can dynamically query plugins and generate the documentation?

@alanz

This comment has been minimized.

Collaborator

alanz commented Jan 4, 2016

My understanding is that this is a kind of magic layer above the plugin
that causes the type stuff to happen. So no great burden on plugin writers.

On Mon, Jan 4, 2016 at 11:19 PM, JP Moresmau notifications@github.com
wrote:

My 2 cents: I understand the requirement, and having swagger integration
will be great, but I fear the complexity for aspiring plugin writers. Heck,
I have no idea what singleton types are or why I should care (-:. Maybe we
should investigate how we can dynamically query plugins and generate the
documentation?


Reply to this email directly or view it on GitHub
#152 (comment)
.

@JPMoresmau

This comment has been minimized.

Collaborator

JPMoresmau commented Jan 4, 2016

OK, understood.

@cocreature

This comment has been minimized.

Collaborator

cocreature commented Jan 4, 2016

@JPMoresmau I share your concern, but let’s take a look how the complexity increases by taking a look at a reasonably complex example: ghcmod.
What has changed?

  • You have to use an underscore in a type signature for PartialTypeSignatures, imho that’s not really more complex.
  • You have to replace buildCommand by buildCommand' which is only necessary because I haven’t gotten around to cleaning it up, so that’s no increase in complexity.
  • You have to use (Proxy :: Proxy "string") instead of "string", now that’s definitely an increase in complexity, but it’s not too bad imho, especially if we document that.
  • You need to use the singleton variants of types, all that means for the user is to use a slightly different name, I also don’t think that increases the complexity noticeably.
  • You need to use Vinyl Recs instead of standard lists, again that’s an increase in complexity, but you don’t need to know anything about how they work or what they are except that the equivalent of : is :& and the equivalent of [] is RNil. Again we should document this, but at least to me (I am obviously biased) it doesn’t look too bad.

Regarding the actual types for use in Leksah. All types should be convertable to the old version (we actually use them still everywhere except for servant) so depending on what exactly you do in Leksah (I haven’t gotten around to looking at the code) it should require very little changes.

@cocreature

This comment has been minimized.

Collaborator

cocreature commented Jan 5, 2016

I think I’m done with the cleanup but I’ve probably forgotten some stuff (that’s why we review PRs :)).

We should definitely wait until @JPMoresmau gets some time to take a look at it from the leksah side, but then we can hopefully merge it rather soon as large PRs such as this one tend to get out of date pretty quickly.

Obviously not everyone is familiar with fancy typelevel magic (in fact I wasn’t really before I tried this) so if you are confused by parts please tell me and I’ll try to explain it to you and add a comment for future readers :)

@JPMoresmau

This comment has been minimized.

Collaborator

JPMoresmau commented Jan 5, 2016

the only change I had to make to get the code in Leksah to compile and work was changed CommandDescriptor to UntaggedCommandDescriptor. So I guess we're good. Unless maybe renaming these two things so that the type alias has the shortest name would be better?

@cocreature

This comment has been minimized.

Collaborator

cocreature commented Jan 6, 2016

@JPMoresmau Thanks for testing it. Let me explain the naming. In most cases the previous name now refers to a parametric version, e.g. CommandDescriptor is parametrized by the type for the contexts and the params. Then there is UntaggedCommandDescriptor which simply instantiates that type with lists and TaggedCommandDescriptor (which I actually forgot, I just added it) which instantiates that type using Vinyl records. I suppose I could rename the parametric one to GenericCommandDescriptor and the one using lists to CommandDescriptor but I’m not really sure if this is better. However if you and @alanz prefer this, I’ll obviously change this.

@alanz

This comment has been minimized.

Collaborator

alanz commented Jan 6, 2016

I think the normal usage case (for plugin writers/IDE integrators) should be the natural name, the internal plumbing names can be longer.

@cocreature

This comment has been minimized.

Collaborator

cocreature commented Jan 6, 2016

@alanz Plugin writers and IDE integrators (which in this case is limited to IDEs written in haskell) use different things here, in particular plugin writers need to use the tagged versions because the types we use in servant have to come from somewhere and the IDEs (meaning Leksah) use the simple types. One of them needs to use a longer name. Note that the data constructor is the same for both types, the only thing different is the type synonym (which doesn’t need to be used, you can always manually expand it). Now I’m not opposed to changing CommandDescriptor to GenericCommandDescriptor, but then we should imho also change the name of the data constructor since it’s confusing if they don’t correspond to the type name. However if we do that it gets even more confusing since then IDEs use CommandDescriptor as a type, but GenericCommandDescriptor as a data constructor. I am not sure if this really improves the situation.

@cocreature

This comment has been minimized.

Collaborator

cocreature commented Jan 6, 2016

Since github doesn’t send mails for new commits: I just updated this pr to be mergable again.

@@ -1,3 +1,7 @@
{-# LANGUAGE InstanceSigs #-}
{-# LANGUAGE UndecidableInstances #-}

This comment has been minimized.

@alanz

alanz Jan 7, 2016

Collaborator

Isn't this a massive danger sign? UndecidableInstances?

This comment has been minimized.

@cocreature

cocreature Jan 7, 2016

Collaborator

Depends on who you ask, I use it fairly often. It is required for some of the magic singletons does. We could disable it and write the singletons manually.

This comment has been minimized.

@cocreature

cocreature Jan 7, 2016

Collaborator

Which would mean that we also need to implement our own concatMap type family or inline that.

This comment has been minimized.

@alanz

alanz Jan 7, 2016

Collaborator

Can we move the singleton definitions into their own module, which has
UndecidableInstances, and the rest of the code does not?

On Thu, Jan 7, 2016 at 6:08 PM, Moritz Kiefer notifications@github.com
wrote:

In hie-base/Haskell/Ide/Engine/PluginTypes.hs
#152 (comment)
:

@@ -1,3 +1,7 @@
+{-# LANGUAGE InstanceSigs #-}
+{-# LANGUAGE UndecidableInstances #-}

Which would mean that we also need to implement our own concatMap type
family or inline that.


Reply to this email directly or view it on GitHub
https://github.com/haskell/haskell-ide-engine/pull/152/files#r49089527.

This comment has been minimized.

@cocreature

cocreature Jan 7, 2016

Collaborator

Will do.

This comment has been minimized.

@alanz

alanz Jan 7, 2016

Collaborator

How hard is that to do?

On Thu, Jan 7, 2016 at 6:06 PM, Moritz Kiefer notifications@github.com
wrote:

In hie-base/Haskell/Ide/Engine/PluginTypes.hs
#152 (comment)
:

@@ -1,3 +1,7 @@
+{-# LANGUAGE InstanceSigs #-}
+{-# LANGUAGE UndecidableInstances #-}

Depends on who you ask, I use it fairly often. It is required for some of
the magic singletons does. We could disable it and write the singletons
manually.


Reply to this email directly or view it on GitHub
https://github.com/haskell/haskell-ide-engine/pull/152/files#r49089216.

This comment has been minimized.

@cocreature

cocreature Jan 7, 2016

Collaborator

Dropping the dependency on singletons or moving the definitions into a separate module?
The latter should be easy. Dropping the dependency on singletons shouldn’t be too hard. The disadvantage is that it introduces some ugly duplication since the definitions for the singletons and the normal data types need to match. Maybe there is also a way to only import a minimal part of singletons which doesn’t require UndecidableInstances.

This comment has been minimized.

@alanz

alanz Jan 7, 2016

Collaborator

I think Singletons are fine, it is more about limiting the scope of the
UndecidableInstances.

Which the people on #ghc tell me are not that bad, but if enabled you can
potentially do stupid things, as you turn off certain checks

On Thu, Jan 7, 2016 at 6:23 PM, Moritz Kiefer notifications@github.com
wrote:

In hie-base/Haskell/Ide/Engine/PluginTypes.hs
#152 (comment)
:

@@ -1,3 +1,7 @@
+{-# LANGUAGE InstanceSigs #-}
+{-# LANGUAGE UndecidableInstances #-}

Dropping the dependency on singletons or moving the definitions into a
separate module?
The latter should be easy. Dropping the dependency on singletons shouldn’t
be too hard. The disadvantage is that it introduces some ugly duplication
since the definitions for the singletons and the normal data types need to
match. Maybe there is also a way to only import a minimal part of
singletons which doesn’t require UndecidableInstances.


Reply to this email directly or view it on GitHub
https://github.com/haskell/haskell-ide-engine/pull/152/files#r49091487.

This comment has been minimized.

@cocreature

cocreature Jan 7, 2016

Collaborator

I just pushed a commit creating two separate modules, one for the singletons, which actually ended up not needing UndecidableInstances but instead there is a different issue since singletons creates some type synonyms which we don’t use so they create unused warnings. To work around this this module simply exports everything. The other module does actually enable UndecidableInstances since there is no way around it with nested type families (which are required here, at least if we want to have somewhat clean code). I also added notes about the singletons in the haddocks of the non singletons types.

This comment has been minimized.

@alanz

alanz Jan 7, 2016

Collaborator

Well, if splitting it doen't localise the UndecidableInstances then perhaps
revert it

On Thu, Jan 7, 2016 at 9:27 PM, Moritz Kiefer notifications@github.com
wrote:

In hie-base/Haskell/Ide/Engine/PluginTypes.hs
#152 (comment)
:

@@ -1,3 +1,7 @@
+{-# LANGUAGE InstanceSigs #-}
+{-# LANGUAGE UndecidableInstances #-}

I just pushed a commit creating two separate modules, one for the
singletons, which actually ended up not needing UndecidableInstances but
instead there is a different issue since singletons creates some type
synonyms which we don’t use so they create unused warnings. To work around
this this module simply exports everything. The other module does actually
enable UndecidableInstances since there is no way around it with nested
type families (which are required here, at least if we want to have
somewhat clean code). I also added notes about the singletons in the
haddocks of the non singletons types.


Reply to this email directly or view it on GitHub
https://github.com/haskell/haskell-ide-engine/pull/152/files#r49114376.

@@ -39,7 +49,14 @@ module Haskell.Ide.Engine.PluginTypes
, IdeResponse(..)
, IdeError(..)
, IdeErrorCode(..)
, SParamDescription(..)

This comment has been minimized.

@alanz

alanz Jan 7, 2016

Collaborator

Perhaps we should add a haddock comment introducing these singleton types

-- ---------------------------------------------------------------------
-- | For a given 'AcceptedContext', define the parameters that are required in
-- the corresponding 'IdeRequest'
contextMapping :: AcceptedContext -> [ParamDescription]

This comment has been minimized.

@alanz

alanz Jan 7, 2016

Collaborator

Does this go away as the info is now encoded in the types?

This comment has been minimized.

@cocreature

cocreature Jan 7, 2016

Collaborator

Not yet, this PR only uses the type info for servant and then drops all the tags and uses the untagged versions. We should definitely try to use those types elsewhere too, but imho this PR is big enough already so I would prefer doing that in multiple steps.

@cocreature cocreature force-pushed the cocreature:servant-fancy branch from 27eb811 to afa5502 Jan 7, 2016

cocreature added a commit that referenced this pull request Jan 7, 2016

Merge pull request #152 from cocreature/servant-fancy
[POC] Move plugin and command names to the typelevel

@cocreature cocreature merged commit df4a854 into haskell:master Jan 7, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@alanz

This comment has been minimized.

Collaborator

alanz commented Jan 7, 2016

Well done, quite an effort

On Thu, Jan 7, 2016 at 10:55 PM, Moritz Kiefer notifications@github.com
wrote:

Merged #152 #152.


Reply to this email directly or view it on GitHub
#152 (comment).

@cocreature

This comment has been minimized.

Collaborator

cocreature commented Jan 7, 2016

Thanks.

@cocreature cocreature referenced this pull request Jan 7, 2016

Closed

Provide REST API explorer #19

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment