-
Notifications
You must be signed in to change notification settings - Fork 564
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
Add support for user defined kinds #2486
Conversation
@@ -95,6 +95,7 @@ convertDecl P.EDValue{..} = Just $ IdeDeclValue $ | |||
IdeValue edValueName edValueType | |||
convertDecl P.EDClass{..} = Just (IdeDeclTypeClass edClassName) | |||
convertDecl P.EDInstance{} = Nothing | |||
convertDecl P.EDKind{} = Nothing |
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 wasn't sure what this was. I'll investigate further to see what actually needs doing here. I assume I'll need to add a new IdeDeclaration
constructor.
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.
@kritzcreek Can you comment?
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.
If you wanted the custom Kinds to show up in completions you'd have to add a new IdeDeclaration
constructor, yep. One question though: Do kinds live in their own name space as well? So can I have a kind, type and identifier with the same name? That would require some more changes to psc-ide
.
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.
Yes they have their own namespace. What kind of changes are required?
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.
Add a dtor to IdeDeclaration in Ide.Types
and fill in identifierFromIdeDeclaration
and completionFromMatch
in Ide.Util
.
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.
Oh sry I read that as though they didn't :D the changes required are in extractSpans
in Ide.SourceFile
where I used an Either
to disambiguate names on the type and value level
I think putting it in
Kind aliases? E.g. |
@@ -19,7 +19,7 @@ import Language.PureScript.AST | |||
import Language.PureScript.Crash | |||
import Language.PureScript.Errors | |||
import Language.PureScript.Names | |||
import Language.PureScript.Sugar.Names.Env |
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.
All the lines that have changed but don't look different in this file are line-ending changes.
If I move |
I think |
@@ -20,6 +22,8 @@ data Kind | |||
| FunKind Kind Kind | |||
-- | Type-level strings | |||
| Symbol | |||
-- | A kind constructor | |||
| KindConstructor (Qualified (ProperName 'KindName)) |
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.
Could we just call this NamedKind
or OtherKind
? Constructor seems to imply that we can have kinds which take other kinds as arguments, which isn't true.
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.
Thanks, I've gone with NamedKind
.
Looks great, thanks! |
One more thing you'll probably need to update is |
@paf31 for updating |
Yes, that sounds right. |
Changing |
Actually, changing |
Okay so I added |
Actually, having thought about this more, I'm not sure why (in which case there would be less work to do here, so you might want to hold off until this is clarified) |
Yep, I mentioned this a while ago in another PR! |
Sorry :) So does that mean we can just get rid of |
Even if I don't change |
Are changes to psc-docs also necessary? E.g. https://github.com/purescript/purescript/blob/master/psc-docs/Tags.hs and Markdown generation. |
I think we need to add stuff to |
Unfortunately I can't build psci and psc-ide-server locally, so I'll have to depend on CI for that. |
Yes, I think so. |
= IdeNameValue Text | ||
| IdeNameType Text | ||
| IdeNameKind Text | ||
deriving (Show, Eq, Ord) |
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.
@kritzcreek is this okay for replacing the use of Either Text Text
that you mentioned?
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.
Maybe replace Name
with NS
or Namespace
but other than that looks great!
Actually I don't think anything needs adding to |
@@ -73,6 +73,7 @@ createBindingGroups moduleName = mapM f <=< handleDecls | |||
valueVerts = map (\d -> (d, declIdent d, usedIdents moduleName d `intersect` allIdents)) values | |||
bindingGroupDecls <- parU (stronglyConnComp valueVerts) (toBindingGroup moduleName) | |||
return $ filter isImportDecl ds ++ | |||
filter isExternKindDecl ds ++ |
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'm slightly confused about this. Without adding in this filter, does that mean that the kind declarations disappear when creating binding groups? Also is this all that is needed here?
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.
Correct
Updated the
Something is wrong :P |
Did you update the |
@paf31 yes I believe
|
Ideally not, but I don't think we should make a special case for the |
-- | Type-level strings | ||
| Symbol | ||
-- | A named kind | ||
| NamedKind (Qualified (ProperName 'KindName)) |
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 Star
and Bang
should be moved into Prim
too?
Eventually, I would like Bang
to be moved into purescript-eff
, but in order to do that, we need to introduce a name for it other than !
(perhaps Effect
?). So how about we move it into Prim
now, allow both !
and Effect
in the parser, and then move it out to purescript-eff
in 0.11?
While we're at it, we can alias *
as Type
.
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.
That sounds great, I should hopefully be able to get this done tonight/tomorrow.
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.
In updating this I've found that I'd missed updating primImports
in Sugar/Names/Env.hs
.
I removed the |
@LiamGoodacre Looks great, but could you please see if it's possible/simple to add documentation for the kinds in Thanks! |
That will require changes to Language.PureScript.Docs.Types to add a new DeclarationInfo constructor for user defined kinds. As far as I can tell, I think that would be fairly straightforward. |
@hdgarrood I had already added a new How does the "Add docs for kinds in Prim" commit look? I haven't written that much for each one. |
I renamed |
Oops :) What you've got there all looks great to me, I agree that the way that you've renamed |
@LiamGoodacre Could you please rebase this? |
Thanks very much! |
Initial work towards #919
Kind declarations:
Import/export kinds:
Use of kinds in various places (see example/passing/ForeignKind/Lib.purs):
Should I also move
Symbol
to use the newKindConstructor
?There are probably a lot more tests I should add, but it's a start.
Any issues, things I've missed, etc?
Thanks 😄