GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — Support deriving via by kl0tl · Pull Request #3824 · purescript/purescript

Conversation

kl0tl commented Mar 27, 2020 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Member

It looks like it works but there’s a few things I need help with:

I had to add cases for ParensInType in various places. This constructor doesn’t usually seem to be present in types but it is in via types. Is it supposed to be somehow removed?
This is perhaps related to the previous point but the error messages I added contain unfortunate line breaks after prettified via types.
I would like to throw a more helpful error than NoInstanceFound when the kinds of the via type and of the instance head last type don’t unify. I tried to elaborate those kinds but elaborating the kind of an application whose first parameter has a polymorphic kind fails.

For instance Data.Functor.Compose.Compose has such a polymorphic kind:

newtype Compose :: ∀ k1 k2. (k1 -> Type) -> (k2 -> k1) -> k2 -> Type

meaning that elaborating the kind of Compose (Either e) Maybe fails when elaborating the application of Compose to Either e. Should I approach this differently?

Also I took the liberty to rename MissingNewtypeSuperclassInstance to MissingSuperclassInstance since this error is thrown for both newtype and via instances, removed the mention of derived superclass instance from its message (since any instance actually qualify) and replaced the InvalidNewtypeInstance error thrown when trying to derive a newtype instance for a nullary type class by a new CannotDeriveNullaryTypeClassInstance error (which is also thrown for via instances).

Close #3302.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

paluh reacted with heart emoji

All reactions

❤️ 1 reaction

natefaubion commented Mar 27, 2020

Copy link

Copy Markdown

Contributor

I had to had cases for ParensInType in various places. This constructor doesn’t usually seem to be present in types but it is in via types. Is it supposed to be somehow removed?

ParensInType is removed as part of operator desugaring (removeParens). You are probably missing a case when traversing types for the deriving via declaration.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

kl0tl force-pushed the deriving-via branch from 4663371 to 6521017 Compare

March 28, 2020 20:31

kl0tl commented Mar 28, 2020

Copy link

Copy Markdown

Member Author

Oh thank you, I added the guilty case 🙇 Also I fixed my error messages and moved most of the deriving via checks into the type checker because I was duplicating a lot of work to infer kinds.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

natefaubion reviewed

Mar 28, 2020

View reviewed changes

Comment thread

lib/purescript-cst/src/Language/PureScript/CST/Parser.y

		'then' { SourceToken _ (TokLowerName [] "then") }
		'true' { SourceToken _ (TokLowerName [] "true") }
		'type' { SourceToken _ (TokLowerName [] "type") }
		'via' { SourceToken _ (TokLowerName [] "via") }

natefaubion Mar 28, 2020

Copy link

Copy Markdown

Contributor

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.

Since this is a contextual keyword, you will need to make sure to add it ident, qualIdent, and label.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

kl0tl Mar 29, 2020

Copy link

Copy Markdown

Member Author

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.

Good catch! Sorry about that, I added a few test cases to cover this.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

kl0tl force-pushed the deriving-via branch from 6521017 to baba10c Compare

March 29, 2020 17:16

kl0tl commented Mar 29, 2020

Copy link

Copy Markdown

Member Author

I managed to ensure the via kind unifies with the kind of the type for which the instance is derived! Elaborating the kinds didn’t work but inferring them after binding all free variables to fresh kinds (like it is done when checking instances declarations) does 🎉

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

kl0tl force-pushed the deriving-via branch 2 times, most recently from 2a3abeb to 25050ef Compare

March 29, 2020 18:19

JordanMartinez mentioned this pull request

Mar 29, 2020

Update syntax to use 'deriving via' syntax JordanMartinez/purescript-jordans-reference#459

Open

paf31 reviewed

Mar 30, 2020

View reviewed changes

Comment thread

src/Language/PureScript/TypeChecker.hs

		, solverDeferErrors = False
		}
		void . flip runStateT M.empty . runWriterT $
		entails solverOptions constraint M.empty []

paf31 Mar 30, 2020

Copy link

Copy Markdown

Contributor

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.

Does this throw away the unsolved constraints?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

paf31 Mar 30, 2020

Copy link

Copy Markdown

Contributor

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, but that's fine because of the current SolverOptions, so the solver won't emit unsolved constraints and instead just fail, right? I guess that means we can't write something like

data T

derive via (...) instance blah :: Coercible X Y => Cls T

i.e. defer Coercible constraints in the instance head, but that's not a big loss, and maybe something that can be solved later?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

kl0tl Mar 30, 2020

Copy link

Copy Markdown

Member Author

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 solver indeed fails when typechecking the instance declaration if the generated constraint can’t be solved but it doesn’t try to solve the instance context, so you can write such instance.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

kl0tl commented

Mar 30, 2020

View reviewed changes

Comment thread

src/Language/PureScript/Errors.hs

		gSimple (MissingNewtypeSuperclassInstance cl1 cl2 ts) = MissingNewtypeSuperclassInstance cl1 cl2 <$> traverse f ts
		gSimple (UnverifiableSuperclassInstance cl1 cl2 ts) = UnverifiableSuperclassInstance cl1 cl2 <$> traverse f ts
		gSimple (InvalidViaType cl ts viaTy) = InvalidViaType cl <$> traverse f ts <*> f viaTy
		gSimple (InvalidViaKind cl ts viaTy viaKind expectedKind) = InvalidViaKind cl <$> traverse f ts <> f viaTy <> f viaKind <*> f expectedKind

kl0tl Mar 30, 2020

Copy link

Copy Markdown

Member Author

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.

Should f be called on kinds here? There’s no case for the KindsDoNotUnify error for instance so maybe it shouldn’t 🤔

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

natefaubion Mar 30, 2020

Copy link

Copy Markdown

Contributor

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 there are cases that are missing it's probably an oversight.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

kl0tl Mar 30, 2020

Copy link

Copy Markdown

Member Author

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 following SimpleErrorMessage constructors hold SourceTypes but aren’t matched:

InfiniteKind
EscapedSkolem
KindsDoNotUnify
MissingClassMember
UserDefinedWarning
QuantificationCheckFailureInType
UnsupportedTypeInKind

Should I add cases for them in this PR? Matching every constructor explicitly would be verbose but could help to prevent this in the future.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

kl0tl force-pushed the deriving-via branch from 25050ef to 1eb3791 Compare

April 6, 2020 19:05

kl0tl mentioned this pull request

Apr 14, 2020

Add a newtype implementing generic instances for use with deriving via purescript-deprecated/purescript-generics-rep#37

Closed


          Support deriving via

b8e4ace

kl0tl force-pushed the deriving-via branch from 1eb3791 to b8e4ace Compare

April 22, 2020 21:52

natefaubion commented Apr 26, 2020

Copy link

Copy Markdown

Contributor

One thing to consider with this syntax is #1120. If we allowed forall for instances, the via clause would be outside of the binder. Should we discuss making via a proper reserved word and put it at the end of the instance? Obviously that ticket isn't a done deal, but I think it's something other maintainers are keen on, so some bike-shedding might be appropriate.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

JordanMartinez mentioned this pull request

Dec 25, 2020

Add UsingGeneric newtype purescript/purescript-prelude#239

Closed

kl0tl mentioned this pull request

Dec 26, 2020

[generics-rep] Newtypes for deriving via purescript/purescript-prelude#241

Open

jrairigh mentioned this pull request

Apr 7, 2021

Allow forall in polymorphic instance heads #1120

Open

rhendric mentioned this pull request

Sep 16, 2022

Add compiler-support for deriving Debug type class #4390

Closed

5 tasks

This was referenced Apr 3, 2026

Add deriving clauses to data and newtype declarations i-am-the-slime/purescript#1

Closed

Add derive clauses to data and newtype declarations #4592

Closed

Reviewers

natefaubion

natefaubion left review comments

+1 more reviewer

paf31

paf31 left review comments

Reviewers whose approvals may not affect merge requirements

At least 1 approving review is required to merge this pull request.

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Support deriving via

Uh oh!

There was an error while loading. Please reload this page.

3 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402520 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

Conversation

kl0tl commented Mar 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

natefaubion commented Mar 27, 2020

Uh oh!

kl0tl commented Mar 28, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kl0tl commented Mar 29, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

natefaubion commented Apr 26, 2020

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

kl0tl commented Mar 27, 2020 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.