-
Notifications
You must be signed in to change notification settings - Fork 95
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
Generate types from metamodel #458
Conversation
This generates all the LSP types from the LSP metamodel, a new machine-readable format they have produced. This requires a pretty formidable amount of TH, but in return we get a hopefully much better maintenance process, and we won't always be behind or wrong as we have been. There are also some nice touches, like on GHC >9.2 we can now actually attach doucmentation to things we generate with TH, so we can transfer all the documentation from the metamodel to Haddock. This is going to result in a _lot_ of churn, enough so that I think it should be a new top-major version of `lsp-types`. In particular: - Various inconsistencies with the spec got fixed. - We aim for scrupulously matching the spec, even when that is weird, so there's a lot more `A |? Null` rather than `Maybe A` than there used to be. - The new version makes heavy use of anonymous records to handle the anonymous structure literals in the spec. - I took a fairly heavy-handed approach to avoiding name clashes in a reasonably future-proof way, but that means that a bunch of names changed, notably the constructors of `Method`/`SMethod`. This still needs a bunch of work: - [ ] `lsp-test` tests are broken for some reason - [ ] More documentation and tidying in `CodeGen` - [ ] Changelog - [ ] Explain how to update the metamodel in the README - [ ] Tidy up the generated documentation so that links go to the generated types not the LSP spec. - [ ] Ensure everything builds on all versions of GHC we support. Plus a bunch of other TODOs in the code. Any help would be welcome, but here are the top things I'm interested in feedback on: 1. How does the new version look? Is the organization okay? I'll try and get some Haddock with the new documentation in place soonish so people can look at that without building it. 2. The gnarly TH. Anyone who wants to dive in is more than welcome, it's not _too_ bad but it would be nice if we could figure out a way to make it less gnarly.
@sjakobi I think you're the main maintainer of the dhall language server, which is one of the main non-HLS users of this package that I know of. I'd really appreciate your thoughts! |
Guide for reviewers:
|
Unfortunately, I'm not very familiar with the |
My take is that you should just do whatever you need to do. We'll figure out a way to adapt downstream |
|
||
-- | Applies formatting to the specified document. | ||
formatDoc :: TextDocumentIdentifier -> FormattingOptions -> Session () | ||
formatDoc doc opts = do | ||
let params = DocumentFormattingParams Nothing doc opts | ||
edits <- getResponseResult <$> request STextDocumentFormatting params | ||
edits <- absorbNull . getResponseResult <$> request SMethod_TextDocumentFormatting params |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know this is lsp-test code but this line pops up a bunch
import Data.Aeson hiding (Null, String) | ||
import qualified Data.Aeson as JSON | ||
import qualified Data.Aeson.TH as JSON |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: some mixture between qualified and unqualified aeson functions in this file
Nothing -> fail $ "no name for " ++ show name | ||
addDocumentation tn documentation since proposed | ||
|
||
ctor <- lift $ makeToplevelName name |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can just use tn
here? Or does it have to be a new name? or does it even matter?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then in genMetaModel
no need to un-mangle names?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nvm it breaks TH - got it
-- | Generate a type declaration corresponding to a top-level named struct. | ||
genStruct :: Structure -> CodeGenM [TH.Dec] | ||
genStruct s@Structure{name, documentation, since, proposed} = do | ||
let structName = name |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: structName is used in one place only
|
||
-- Result type is Maybe unless X allows custom values, in which case we can always interpret | ||
-- a base value as a custom member | ||
-- valueToX :: <base type> -> Maybe X |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we want a partial function for non-custom enums?
"both" -> pure Both | ||
t -> fail $ "unknown message direction " ++ show t | ||
|
||
data BaseTypeName = URI | DocumentUri | Integer | UInteger | Decimal | RegExp | String | Boolean | Null |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: URI -> Uri :) Unless there is some name clashing?
n <- lift $ TH.newName "arg" | ||
pairE <- case optional of | ||
Just True -> lift [| $(TH.litE $ TH.stringL $ T.unpack name) .=? $(TH.varE n) |] | ||
_ -> lift [| [$(TH.litE $ TH.stringL $ T.unpack name) J..= $(TH.varE n)] |] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The spec defining "optional" to be False if it's omitted is confusing and weird... but it's in the spec so
extends' = fromMaybe [] extends | ||
mixins' = fromMaybe [] mixins | ||
supertypes = extends' ++ mixins' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if it's ever possible for an extended type
or mixin
type to have duplicate fields? Is it worth de-duping superProps
(aside from local name de-duping)
|
||
{- | ||
TODO: this is out-of-date/needs an audit | ||
TODO: can we generate this? process the 'since' annotations in the metamodel? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like a good idea to generate this as well. I'm going cross-eyed looking at it. I can give it a shot if you want
I honestly, don't think there is much to change. The TH generation seems sensible, I don't think there's much to change honestly. Codegen is pretty much straightforward, no matter what language you are using. I made some general code nit picks that honestly you can just ignore. There were one or two actual suggestions that I think are worth it. Otherwise lgtm. |
|
||
data Request = Request | ||
{ method :: Text | ||
, params :: Maybe Type -- typescript says it can be [Type], but it never is so whatever |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd argue this should match the spec, better to match the spec and reduce churn later imo
sanitizeName n = | ||
-- Names can't start with underscores! Replace that with a 'U' for lack | ||
-- of a better idea | ||
let n' = if "_" `T.isPrefixOf` n then T.cons 'U' $ T.tail n else n |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about Abstract
or Base
or Internal
or something like that. Looks like most usages are "abstract" classes in the traditional OOP sense.
Any update here? I'm going to address some lsp-test parts, some new features from 3.16 spec attracted me. |
I have a partially-rewritten version that generates Haskell source instead. I wasn't able to get this version to work with GHC 9, and the thought of trying to make it work with multiple GHC versions made me scared. So I decided to switch to generating source. I need to finish it, basically... |
Closing in favour of #478 |
This generates all the LSP types from the LSP metamodel, a new machine-readable format they have produced.
This requires a pretty formidable amount of TH, but in return we get a hopefully much better maintenance process, and we won't always be behind or wrong as we have been. There are also some nice touches, like on GHC >9.2
we can now actually attach doucmentation to things we generate with
TH, so we can transfer all the documentation from the metamodel to Haddock.
This is going to result in a lot of churn, enough so that I think it should be a new top-major version of
lsp-types
. In particular:A |? Null
rather thanMaybe A
than there used to be.Method
/SMethod
.This still needs a bunch of work:
lsp-test
tests are broken for some reasonCodeGen
Plus a bunch of other TODOs in the code. Any help would be welcome, but here are the top things I'm interested in feedback on: