Rebasing generic work #394

treeowl · 2022-02-23T04:55:09Z

Rebase in progress of #316.

treeowl · 2022-02-24T02:09:33Z

I don't know how to set up ormolu for formatting, and I don't really have time to futz with that right now. Can someone help me out?

treeowl · 2022-02-24T02:11:26Z

@aspiwack aside from the formatting thing, I think this is pretty much ready. It doesn't remove all the orphans, but that can be done in another PR or two.

tbagrel1 · 2022-02-24T07:55:55Z

I don't know how to set up ormolu for formatting, and I don't really have time to futz with that right now. Can someone help me out?

You should be able to format with ./format.sh at the root of the repo. It uses stack to fetch, build and exec ormolu on the codebase.

aspiwack · 2022-02-24T07:58:32Z

If you have Nix installed, you can $ nix-shell --run "./format,sh". Alternatively, if you have Stack installed, you can ./format.sh.

I pushed a formatting commit.

@tbagrel1 This is a pretty big patch, please give it a review, I'll do the same myself.

tbagrel1

Just a bit of nitpicking. Thank you really much for this impressive work!

src/Control/Functor/Linear/Internal/Class.hs

src/Data/Functor/Linear/Internal/Functor.hs

src/Data/Unrestricted/Linear/Internal/Dupable.hs

src/Data/Unrestricted/Linear/Internal/Instances.hs

src/Data/Unrestricted/Linear/Internal/Dupable.hs

src/Data/Unrestricted/Linear/Internal/Movable.hs

aspiwack

(I've reviewed only about half way through, but I won't have time to resume for a while, so posting what I have)

linear-base.cabal

src/Control/Functor/Linear/Internal/Class.hs

src/Data/Functor/Linear/Internal/Applicative.hs

src/Data/Functor/Linear/Internal/Functor.hs

src/Data/Functor/Linear/Internal/Traversable.hs

tbagrel1 · 2022-02-24T08:36:11Z

@aspiwack Do you remember the motivations behind the difference of organisation between Data.Functor.Linear.xxx modules and Control.Functor.Linear.xxx modules?

treeowl · 2022-02-24T10:10:22Z

@aspiwack , you seem to be fighting a long term trend to prefer explicit and qualified imports to hiding in most cases. Why is that? The former are more robust in the face of additions to the imported modules.

aspiwack · 2022-02-24T12:33:44Z

@aspiwack , you seem to be fighting a long term trend to prefer explicit and qualified imports to hiding in most cases. Why is that? The former are more robust in the face of additions to the imported modules.

They are more robust, but in practice they cost more of my time. My inclination is to use qualified import as much as is reasonable (which will be easier when the Local Module proposal comes to fruition). But I prefer figuring out the occasional error, than preemptively making import list management something I have to care about on a daily basis.

aspiwack · 2022-02-24T12:35:36Z

@aspiwack Do you remember the motivations behind the difference of organisation between Data.Functor.Linear.xxx modules and Control.Functor.Linear.xxx modules?

I think @Divesh-Otwani found it easier to document modules when they were split into smaller chunks for me. I don't care much either way as long as we have a reasonably small list of modules to import in the user-facing land.

treeowl · 2022-02-24T21:58:37Z

Note: when this is merged, it would be good to give @sjoerdvisscher credit for the substantial work he did on it before I showed up. As I recall, his most interesting contribution was generic derivation of control functors. I'm not sure how best to do that in the context of git with such a wild rebase.

treeowl · 2022-02-24T22:50:19Z

@aspiwack ormolu chokes doing something or other with dependencies (constraints and hedgehog) that have CPP in their module export lists when I use the stack version. The nix thing is still compiling the world.

treeowl · 2022-02-25T08:26:19Z

The nix formatting approach fails for me too.

tbagrel1 · 2022-02-25T08:39:49Z

@aspiwack ormolu chokes doing something or other with dependencies (constraints and hedgehog) that have CPP in their module export lists when I use the stack version. The nix thing is still compiling the world.

I formatted it. I got no issues with ormolu (but I already had it installed through format.sh a long time ago, so maybe there is something which prevents ormolu installation via stack now?).

tbagrel1 · 2022-02-25T14:56:22Z

Closes #316 .

aspiwack · 2022-03-07T13:00:30Z

Note: when this is merged, it would be good to give @sjoerdvisscher credit for the substantial work he did on it before I showed up. As I recall, his most interesting contribution was generic derivation of control functors. I'm not sure how best to do that in the context of git with such a wild rebase.

I believe that if you add the line

Co-authored-by: Sjoerd Visscher <firstname.name@tweag.io>

To the relevant commits' messages, then Sjoerd will appear – at least in Github – as a coauthor of the commits.

aspiwack · 2022-03-07T13:02:45Z

I think it's specifically at the end of the commit message. Github's documentation is here.

aspiwack

There are only a few points to address to get this merged. @treeowl can you do that this week? If so we can include it in the next release.

I really like this. Thanks to both of you and @sjoerdvisscher for the great work that came into this.

aspiwack · 2022-02-24T12:56:41Z

src/Prelude/Linear/Internal.hs

+lcoerce :: forall a b. Coercible a b => a %1 -> b
+lcoerce = coerce ((\x -> x) :: a %1 -> a)


Very nice 🙂

Glad you like it ;-)

linear-base.cabal

src/Data/Unrestricted/Linear/Internal/Dupable.hs

src/Control/Functor/Linear/Internal/Class.hs

aspiwack · 2022-03-14T10:40:07Z

src/Data/Unrestricted/Linear/Internal/Dupable.hs

+-- @deriving via 'Generically'@. Note that at present this mechanism
+-- can have performance problems for recursive parameterized types.
+-- Specifically, the methods will not specialize to underlying 'Dupable'
+-- instances.


I don't understand the performance problem that you are pointing at?

@aspiwack, the underlying Dupable dictionary ends up being a (static) argument to a recursive function that doesn't get an unfolding. So the underlying duplication function will never be inlined, no matter how small it is. Probably not the biggest problem for Dupable; probably a bigger problem for Traversable, where traverse won't specialize to a particular Applicative, let alone a particular function to traverse with. See https://gitlab.haskell.org/ghc/ghc/-/issues/21131

I've linked to the GHC issue from the docs.

src/Prelude/Linear/Unsatisfiable.hs

aspiwack · 2022-03-15T18:09:19Z

Thanks @tbagrel1 for taking care of the small issues. @treeowl do you have time to address the couple of request for extra comments? In either case, I plan to merge this tomorrow, and then we make a release :slightly_smiling_face: .

Co-authored-by: David Feuer <David.Feuer@gmail.com>

To avoid orphans, put the generic deriving machinery with the `Traversable` class.

- `Consumable`, `Dupable` and `Movable` uniformly implemented for tuples up to n=5 - Fix some names/imports according to the project conventions

treeowl · 2022-03-16T05:00:07Z

I think everything's sorted now.

treeowl · 2022-03-16T05:00:39Z

(Unless I've broken some ormolu junk again, in which case I imagine you can fix it.)

aspiwack · 2022-03-16T07:21:31Z

Thanks!

I'm guessing the dance around unsatisfiable is necessary to make it representation-polymorphic. I don't have much of a grasp of why (or why the previous implementation worked). I'd be glad if you gave a few words about it in the thread.

treeowl · 2022-03-16T12:18:59Z

Thanks!

I'm guessing the dance around unsatisfiable is necessary to make it representation-polymorphic. I don't have much of a grasp of why (or why the previous implementation worked). I'd be glad if you gave a few words about it in the thread.

Yes, partly to make it representation polymorphic to match what the GHC built-in version will do as closely as we can. The previous implementation had

unsatisfiable' :: Bottom => a

This was used at type

unsatisfiable' :: Bottom  => (##) -> q

where q is the actual desired result type.

What's the rest of the dance for? The "obvious" thing is to give unsatisfiable an Unsatisfiable e constraint. But this would not be ergonomical at all, and would not match the "magical" behavior of the future built-in version. Why? Because each invocation of unsatisfiable would then have to specify the type error, completely uselessly. By using Bottom, we avoid caring, at the call site, why we couldn't possibly have gotten there.

treeowl · 2022-10-11T09:13:54Z

Yes, I'll try to do it tonight. Lots of things going on!

…

On Tue, Mar 15, 2022, 2:09 PM Arnaud Spiwack ***@***.***> wrote: Thanks @tbagrel1 <https://github.com/tbagrel1> for taking care of the small issues. @treeowl <https://github.com/treeowl> do you have time to address the couple of request for extra comments? In either case, I plan to merge this tomorrow, and then we make a release 🙂 . — Reply to this email directly, view it on GitHub <#394 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAOOF7P4SAUSJCZGO3JYWADVADG5VANCNFSM5PDJKA5Q> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>. You are receiving this because you were mentioned.Message ID: ***@***.***>

treeowl marked this pull request as draft February 23, 2022 04:55

treeowl mentioned this pull request Feb 23, 2022

Add deriving via Generically/Generically1 #316

Closed

treeowl marked this pull request as ready for review February 24, 2022 00:36

treeowl force-pushed the generically branch from 89bb1ef to 107d893 Compare February 24, 2022 02:05

tbagrel1 reviewed Feb 24, 2022

View reviewed changes

aspiwack reviewed Feb 24, 2022

View reviewed changes

This was referenced Feb 25, 2022

Try to get rid of Instances internal modules #396

Open

Add generic implementation and generic deriving for linear Semigroup and Monoid #371

Open

tbagrel1 added this to the v0.2.0 milestone Feb 25, 2022

aspiwack reviewed Mar 14, 2022

View reviewed changes

treeowl added 3 commits March 14, 2022 14:45

Unsatisfiable

23632d0

Generically, data functor

c3e775c

Data applicative

87641ad

tbagrel1 force-pushed the generically branch from 6d7086d to 941b83f Compare March 14, 2022 14:52

sjoerdvisscher and others added 14 commits March 16, 2022 00:53

Control functors/applicatives

c28ed2b

Co-authored-by: David Feuer <David.Feuer@gmail.com>

Merge traversable modules

6029290

To avoid orphans, put the generic deriving machinery with the `Traversable` class.

Traversable instances

5d18192

Consumable

6faf3e3

Dupable

6b99d44

Movable

42209f4

Remove some orphans; use more coercions

d11dfee

Make it compile

73b528d

Format

ca8cd51

Address some comments

be6cd6e

More coercions

96b8040

Fix formatting

bfa6163

Naming convention fixes + Classes for tuples up to n=5

b8d04c2

- `Consumable`, `Dupable` and `Movable` uniformly implemented for tuples up to n=5 - Fix some names/imports according to the project conventions

Simpler unsatisfiable

d46dbb5

treeowl force-pushed the generically branch from 9354182 to d46dbb5 Compare March 16, 2022 04:55

Small comment

c6dcb70

Fix formatting

9b3127e

aspiwack approved these changes Mar 16, 2022

View reviewed changes

tbagrel1 merged commit 0841c91 into tweag:master Mar 16, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rebasing generic work #394

Rebasing generic work #394

treeowl commented Feb 23, 2022

treeowl commented Feb 24, 2022

treeowl commented Feb 24, 2022

tbagrel1 commented Feb 24, 2022

aspiwack commented Feb 24, 2022

tbagrel1 left a comment

aspiwack left a comment

tbagrel1 commented Feb 24, 2022

treeowl commented Feb 24, 2022

aspiwack commented Feb 24, 2022

aspiwack commented Feb 24, 2022

treeowl commented Feb 24, 2022

treeowl commented Feb 24, 2022

treeowl commented Feb 25, 2022

tbagrel1 commented Feb 25, 2022

tbagrel1 commented Feb 25, 2022

aspiwack commented Mar 7, 2022

aspiwack commented Mar 7, 2022

aspiwack left a comment

aspiwack Feb 24, 2022

treeowl Mar 16, 2022

aspiwack Mar 14, 2022

treeowl Mar 16, 2022

treeowl Mar 16, 2022

aspiwack commented Mar 15, 2022

treeowl commented Mar 16, 2022

treeowl commented Mar 16, 2022

aspiwack commented Mar 16, 2022

treeowl commented Mar 16, 2022

treeowl commented Oct 11, 2022 via email

		lcoerce :: forall a b. Coercible a b => a %1 -> b
		lcoerce = coerce ((\x -> x) :: a %1 -> a)

Rebasing generic work #394

Rebasing generic work #394

Conversation

treeowl commented Feb 23, 2022

treeowl commented Feb 24, 2022

treeowl commented Feb 24, 2022

tbagrel1 commented Feb 24, 2022

aspiwack commented Feb 24, 2022

tbagrel1 left a comment

Choose a reason for hiding this comment

aspiwack left a comment

Choose a reason for hiding this comment

tbagrel1 commented Feb 24, 2022

treeowl commented Feb 24, 2022

aspiwack commented Feb 24, 2022

aspiwack commented Feb 24, 2022

treeowl commented Feb 24, 2022

treeowl commented Feb 24, 2022

treeowl commented Feb 25, 2022

tbagrel1 commented Feb 25, 2022

tbagrel1 commented Feb 25, 2022

aspiwack commented Mar 7, 2022

aspiwack commented Mar 7, 2022

aspiwack left a comment

Choose a reason for hiding this comment

aspiwack Feb 24, 2022

Choose a reason for hiding this comment

treeowl Mar 16, 2022

Choose a reason for hiding this comment

aspiwack Mar 14, 2022

Choose a reason for hiding this comment

treeowl Mar 16, 2022

Choose a reason for hiding this comment

treeowl Mar 16, 2022

Choose a reason for hiding this comment

aspiwack commented Mar 15, 2022

treeowl commented Mar 16, 2022

treeowl commented Mar 16, 2022

aspiwack commented Mar 16, 2022

treeowl commented Mar 16, 2022

treeowl commented Oct 11, 2022 via email