Re: Fixing type synonyms to Uniq(D)FM newtypes

[Andreas Klebinger]

My main motivation for going with a phantom type over newtypes was that
it makes it easier to use in
an adhoc fashion without giving up type safety.

As a second benefit it seemed a lot easier to implement.

Cheers
Andreas



George Colpitts schrieb am 24.06.2020 um 00:40:

I read the email thread you refer to but it doesn't seem to explain
why you went with solution 2. If you think it worthwhile can you
explain here why you chose solution 2?

On Tue, Jun 23, 2020 at 6:55 PM Andreas Klebinger
mailto:klebinger.andr...@gmx.at>> wrote:

There was a discussion about making UniqFM typed for the keys here a
while ago.
(https://mail.haskell.org/pipermail/ghc-devs/2020-January/018451.html
and following)

I wrote up an MR for one possible approach here:
https://gitlab.haskell.org/ghc/ghc/-/merge_requests/3577

It implements solution 2 from that discussion.

Just while getting the patch to typecheck I've already seen a
number of
cases where this increased
readability of the code quite a bit so I think it's a good
improvement.

If there are strong objections to this solution let me know. In that
case I'm happy to abandon the patch.
If not I will clean it up and get it ready for merging.


___
ghc-devs mailing list
ghc-devs@haskell.org 
http://mail.haskell.org/cgi-bin/mailman/listinfo/ghc-devs



___
ghc-devs mailing list
ghc-devs@haskell.org
http://mail.haskell.org/cgi-bin/mailman/listinfo/ghc-devs

Re: Fixing type synonyms to Uniq(D)FM newtypes

[George Colpitts]

I read the email thread you refer to but it doesn't seem to explain why you
went with solution 2. If you think it worthwhile can you explain here why
you chose solution 2?

On Tue, Jun 23, 2020 at 6:55 PM Andreas Klebinger 
wrote:

> There was a discussion about making UniqFM typed for the keys here a
> while ago.
> (https://mail.haskell.org/pipermail/ghc-devs/2020-January/018451.html
> and following)
>
> I wrote up an MR for one possible approach here:
> https://gitlab.haskell.org/ghc/ghc/-/merge_requests/3577
>
> It implements solution 2 from that discussion.
>
> Just while getting the patch to typecheck I've already seen a number of
> cases where this increased
> readability of the code quite a bit so I think it's a good improvement.
>
> If there are strong objections to this solution let me know. In that
> case I'm happy to abandon the patch.
> If not I will clean it up and get it ready for merging.
>
>
> ___
> ghc-devs mailing list
> ghc-devs@haskell.org
> http://mail.haskell.org/cgi-bin/mailman/listinfo/ghc-devs
>
___
ghc-devs mailing list
ghc-devs@haskell.org
http://mail.haskell.org/cgi-bin/mailman/listinfo/ghc-devs

Re: Fixing type synonyms to Uniq(D)FM newtypes

[Ben Gamari]

Richard Eisenberg  writes:

> One advantage of the phantom-parameter approach is that it allows for nice 
> polymorphism.
>
>> lookupEnv :: Uniquable dom => UniqFM dom rng -> dom -> Maybe rng
>
> Now, we don't need lookupVarEnv separately from lookupNameEnv, but we
> get the type-checking for free.
>
This is true but some consider the fact that the function name captures
the environment type to be a good thing. I don't have a strong opinion
one way of the other on this.

Cheers,

- Ben


signature.asc
Description: PGP signature
___
ghc-devs mailing list
ghc-devs@haskell.org
http://mail.haskell.org/cgi-bin/mailman/listinfo/ghc-devs

Re: Fixing type synonyms to Uniq(D)FM newtypes

[Richard Eisenberg]

One advantage of the phantom-parameter approach is that it allows for nice 
polymorphism.

> lookupEnv :: Uniquable dom => UniqFM dom rng -> dom -> Maybe rng

Now, we don't need lookupVarEnv separately from lookupNameEnv, but we get the 
type-checking for free.

I agree with Ben about the fact that newtypes have their own advantages.

I don't have much of an opinion, in the end.

Richard

> On Jan 14, 2020, at 10:31 AM, Ben Gamari  wrote:
> 
> Matthew Pickering  writes:
> 
>> Can someone explain the benefit of the newtype wrappers over the
>> phantom type parameter approach?
>> 
>> In my mind adding a phantom type parameter to `UniqFM` solves the
>> issue entirely but will result in less code churn and follows the
>> example from the existing map data types from containers.
>> 
> I would say the same of newtype wrappers; afterall, we already have a
> convention of using the "specialised" type synonyms and their functions
> instead of UniqFM directly where possible. Turning VarEnv, etc. into
> newtypes likely touch little code outside of the modules where they are
> defined.
> 
> Which approach is preferable is really a question of what degree of
> encapsulation we want. The advantage of making, e.g., VarEnv a newtype
> is that our use of Uniques remains an implementation detail (which it
> is, in my opinion). We are then in principle free to change the
> representation of VarEnv down the road.
> 
> Of course, in practice it is hard to imagine GHC moving away from
> uniques for things like VarEnv. However, properly encapsulating them
> seems like good engineering practice and incurs very little cost
> (especially given our current conventions).
> 
> Cheers,
> 
> - Ben
> 
> ___
> ghc-devs mailing list
> ghc-devs@haskell.org
> http://mail.haskell.org/cgi-bin/mailman/listinfo/ghc-devs

___
ghc-devs mailing list
ghc-devs@haskell.org
http://mail.haskell.org/cgi-bin/mailman/listinfo/ghc-devs

Re: Fixing type synonyms to Uniq(D)FM newtypes

[Ben Gamari]

Matthew Pickering  writes:

> Can someone explain the benefit of the newtype wrappers over the
> phantom type parameter approach?
>
> In my mind adding a phantom type parameter to `UniqFM` solves the
> issue entirely but will result in less code churn and follows the
> example from the existing map data types from containers.
>
I would say the same of newtype wrappers; afterall, we already have a
convention of using the "specialised" type synonyms and their functions
instead of UniqFM directly where possible. Turning VarEnv, etc. into
newtypes likely touch little code outside of the modules where they are
defined.

Which approach is preferable is really a question of what degree of
encapsulation we want. The advantage of making, e.g., VarEnv a newtype
is that our use of Uniques remains an implementation detail (which it
is, in my opinion). We are then in principle free to change the
representation of VarEnv down the road.

Of course, in practice it is hard to imagine GHC moving away from
uniques for things like VarEnv. However, properly encapsulating them
seems like good engineering practice and incurs very little cost
(especially given our current conventions).

Cheers,

- Ben



signature.asc
Description: PGP signature
___
ghc-devs mailing list
ghc-devs@haskell.org
http://mail.haskell.org/cgi-bin/mailman/listinfo/ghc-devs

Re: Fixing type synonyms to Uniq(D)FM newtypes

[Matthew Pickering]

Can someone explain the benefit of the newtype wrappers over the
phantom type parameter approach?

In my mind adding a phantom type parameter to `UniqFM` solves the
issue entirely but will result in less code churn and follows the
example from the existing map data types from containers.

Cheers,

Matt

On Tue, Jan 14, 2020 at 10:02 AM Ben Gamari  wrote:
>
> Ömer Sinan Ağacan  writes:
>
> > Hi,
> >
> > UniqFM and UniqDFM types are basically maps from Uniques to other stuff. 
> > Most of
> > the time we don't actually map Uniques but instead map things like Vars or
> > Names. For those we have types like VarEnv, NameEnv, FastStringEnv, ... 
> > which
> > are defined as type synonyms to UniqFM or UniqDFM, and operations are 
> > defined
> > like
> >
> > extendFsEnv = addToUFM
> > extendNameEnv = addToUFM
> > extendVarEnv = addToUFM
> >
> > This causes problems when I have multiple Uniquables in scope and use the 
> > wrong
> > one to update an environment because the program type checks and does the 
> > wrong
> > thing in runtime. An example is #17667 where I did
> >
> > delVarEnv env name
> >
> > where `name :: Name`. I shouldn't be able to remove a Name from a Var env 
> > but
> > this currently type checks.
> >
> At first I was a bit confused at how this could possibly typecheck.
> Afterall, delVarEnv has type,
>
> VarEnv a -> Var -> VarEnv a
>
> which seems quite reasonable and should correctly reject the application
> to a Name as given in Omer's example. However, the mistake in #17667
> is actually that he wrote,
>
> delVarEnv env name
>
> instead of
>
> delNameEnv env (varName var)
>
> That is, because `VarEnv a ~ NameEnv a` one can easily mix up a
> NameEnv with a VarEnv and not get a compile-time error. I can see how
> this could be a nasty bug to track down.
>
>
> > Two solutions proposed:
> >
> > - Make these env types newtypes instead of type synonyms.
> > - Add a phantom type parameter to UniqFM/UniqDFM.
> >
> IIRC this has been suggested before. I, for one, see the value in this
> and certainly wouldn't be opposed to either of these options, although
> would weakly favor the former over the latter.
>
> Cheers,
>
> - Ben
> ___
> ghc-devs mailing list
> ghc-devs@haskell.org
> http://mail.haskell.org/cgi-bin/mailman/listinfo/ghc-devs
___
ghc-devs mailing list
ghc-devs@haskell.org
http://mail.haskell.org/cgi-bin/mailman/listinfo/ghc-devs

Re: Fixing type synonyms to Uniq(D)FM newtypes

[Ben Gamari]

Ömer Sinan Ağacan  writes:

> Hi,
>
> UniqFM and UniqDFM types are basically maps from Uniques to other stuff. Most 
> of
> the time we don't actually map Uniques but instead map things like Vars or
> Names. For those we have types like VarEnv, NameEnv, FastStringEnv, ... which
> are defined as type synonyms to UniqFM or UniqDFM, and operations are defined
> like
>
> extendFsEnv = addToUFM
> extendNameEnv = addToUFM
> extendVarEnv = addToUFM
>
> This causes problems when I have multiple Uniquables in scope and use the 
> wrong
> one to update an environment because the program type checks and does the 
> wrong
> thing in runtime. An example is #17667 where I did
>
> delVarEnv env name
>
> where `name :: Name`. I shouldn't be able to remove a Name from a Var env but
> this currently type checks.
>
At first I was a bit confused at how this could possibly typecheck.
Afterall, delVarEnv has type,

VarEnv a -> Var -> VarEnv a

which seems quite reasonable and should correctly reject the application
to a Name as given in Omer's example. However, the mistake in #17667
is actually that he wrote,

delVarEnv env name

instead of

delNameEnv env (varName var)

That is, because `VarEnv a ~ NameEnv a` one can easily mix up a
NameEnv with a VarEnv and not get a compile-time error. I can see how
this could be a nasty bug to track down.


> Two solutions proposed:
>
> - Make these env types newtypes instead of type synonyms.
> - Add a phantom type parameter to UniqFM/UniqDFM.
>
IIRC this has been suggested before. I, for one, see the value in this
and certainly wouldn't be opposed to either of these options, although
would weakly favor the former over the latter.

Cheers,

- Ben


signature.asc
Description: PGP signature
___
ghc-devs mailing list
ghc-devs@haskell.org
http://mail.haskell.org/cgi-bin/mailman/listinfo/ghc-devs

Re: Fixing type synonyms to Uniq(D)FM newtypes

[Spiwack, Arnaud]

In a way, if these types need to exist at all, they probably should be
newtypes. That being said, I'm pretty sure that the current APIs are
incomplete, so turning these into newtypes may be, in fact, quite a bit of
work.

But if we are starting this discussion, I'd like to understand first why
all these types exist at all? Why not use `UniqFM` everywhere?

/Arnaud

On Tue, Jan 14, 2020 at 7:16 AM Ömer Sinan Ağacan 
wrote:

> > but I still don't quite understand the motivation
>
> I give a concrete example (something that happened to me that I had to
> debug in
> runtime) in the issue I linked in my original post.
>
> > For example, delVarEnv will only work with a Var, not a Name.
>
> delVarEnv will happily accept a NameEnv in its first argument, which is the
> problem I was trying to describe.
>
> Ömer
>
> Richard Eisenberg , 14 Oca 2020 Sal, 01:55 tarihinde
> şunu yazdı:
> >
> > I'd be fine with making these newtypes, but I still don't quite
> understand the motivation. Note that the specialized functions for the
> different instances of UniqFM all have type signatures. For example,
> delVarEnv will only work with a Var, not a Name.
> >
> > Was there a different scenario that you want to avoid?
> >
> > Thanks,
> > Richard
> >
> > > On Jan 13, 2020, at 9:10 AM, Ömer Sinan Ağacan 
> wrote:
> > >
> > > Hi,
> > >
> > > UniqFM and UniqDFM types are basically maps from Uniques to other
> stuff. Most of
> > > the time we don't actually map Uniques but instead map things like
> Vars or
> > > Names. For those we have types like VarEnv, NameEnv, FastStringEnv,
> ... which
> > > are defined as type synonyms to UniqFM or UniqDFM, and operations are
> defined
> > > like
> > >
> > >extendFsEnv = addToUFM
> > >extendNameEnv = addToUFM
> > >extendVarEnv = addToUFM
> > >
> > > This causes problems when I have multiple Uniquables in scope and use
> the wrong
> > > one to update an environment because the program type checks and does
> the wrong
> > > thing in runtime. An example is #17667 where I did
> > >
> > >delVarEnv env name
> > >
> > > where `name :: Name`. I shouldn't be able to remove a Name from a Var
> env but
> > > this currently type checks.
> > >
> > > Two solutions proposed:
> > >
> > > - Make these env types newtypes instead of type synonyms.
> > > - Add a phantom type parameter to UniqFM/UniqDFM.
> > >
> > > If you could share your opinion on how to fix this I'd like to fix
> this soon.
> > >
> > > Personally I prefer (1) because it looks simpler but I'd be happy with
> > > (2) as well.
> > >
> > > Ömer
> > > ___
> > > ghc-devs mailing list
> > > ghc-devs@haskell.org
> > > http://mail.haskell.org/cgi-bin/mailman/listinfo/ghc-devs
> >
> ___
> ghc-devs mailing list
> ghc-devs@haskell.org
> http://mail.haskell.org/cgi-bin/mailman/listinfo/ghc-devs
>
___
ghc-devs mailing list
ghc-devs@haskell.org
http://mail.haskell.org/cgi-bin/mailman/listinfo/ghc-devs

Re: Fixing type synonyms to Uniq(D)FM newtypes

[Ömer Sinan Ağacan]

> but I still don't quite understand the motivation

I give a concrete example (something that happened to me that I had to debug in
runtime) in the issue I linked in my original post.

> For example, delVarEnv will only work with a Var, not a Name.

delVarEnv will happily accept a NameEnv in its first argument, which is the
problem I was trying to describe.

Ömer

Richard Eisenberg , 14 Oca 2020 Sal, 01:55 tarihinde
şunu yazdı:
>
> I'd be fine with making these newtypes, but I still don't quite understand 
> the motivation. Note that the specialized functions for the different 
> instances of UniqFM all have type signatures. For example, delVarEnv will 
> only work with a Var, not a Name.
>
> Was there a different scenario that you want to avoid?
>
> Thanks,
> Richard
>
> > On Jan 13, 2020, at 9:10 AM, Ömer Sinan Ağacan  wrote:
> >
> > Hi,
> >
> > UniqFM and UniqDFM types are basically maps from Uniques to other stuff. 
> > Most of
> > the time we don't actually map Uniques but instead map things like Vars or
> > Names. For those we have types like VarEnv, NameEnv, FastStringEnv, ... 
> > which
> > are defined as type synonyms to UniqFM or UniqDFM, and operations are 
> > defined
> > like
> >
> >extendFsEnv = addToUFM
> >extendNameEnv = addToUFM
> >extendVarEnv = addToUFM
> >
> > This causes problems when I have multiple Uniquables in scope and use the 
> > wrong
> > one to update an environment because the program type checks and does the 
> > wrong
> > thing in runtime. An example is #17667 where I did
> >
> >delVarEnv env name
> >
> > where `name :: Name`. I shouldn't be able to remove a Name from a Var env 
> > but
> > this currently type checks.
> >
> > Two solutions proposed:
> >
> > - Make these env types newtypes instead of type synonyms.
> > - Add a phantom type parameter to UniqFM/UniqDFM.
> >
> > If you could share your opinion on how to fix this I'd like to fix this 
> > soon.
> >
> > Personally I prefer (1) because it looks simpler but I'd be happy with
> > (2) as well.
> >
> > Ömer
> > ___
> > ghc-devs mailing list
> > ghc-devs@haskell.org
> > http://mail.haskell.org/cgi-bin/mailman/listinfo/ghc-devs
>
___
ghc-devs mailing list
ghc-devs@haskell.org
http://mail.haskell.org/cgi-bin/mailman/listinfo/ghc-devs

Re: Fixing type synonyms to Uniq(D)FM newtypes

[Richard Eisenberg]

I'd be fine with making these newtypes, but I still don't quite understand the 
motivation. Note that the specialized functions for the different instances of 
UniqFM all have type signatures. For example, delVarEnv will only work with a 
Var, not a Name.

Was there a different scenario that you want to avoid?

Thanks,
Richard

> On Jan 13, 2020, at 9:10 AM, Ömer Sinan Ağacan  wrote:
> 
> Hi,
> 
> UniqFM and UniqDFM types are basically maps from Uniques to other stuff. Most 
> of
> the time we don't actually map Uniques but instead map things like Vars or
> Names. For those we have types like VarEnv, NameEnv, FastStringEnv, ... which
> are defined as type synonyms to UniqFM or UniqDFM, and operations are defined
> like
> 
>extendFsEnv = addToUFM
>extendNameEnv = addToUFM
>extendVarEnv = addToUFM
> 
> This causes problems when I have multiple Uniquables in scope and use the 
> wrong
> one to update an environment because the program type checks and does the 
> wrong
> thing in runtime. An example is #17667 where I did
> 
>delVarEnv env name
> 
> where `name :: Name`. I shouldn't be able to remove a Name from a Var env but
> this currently type checks.
> 
> Two solutions proposed:
> 
> - Make these env types newtypes instead of type synonyms.
> - Add a phantom type parameter to UniqFM/UniqDFM.
> 
> If you could share your opinion on how to fix this I'd like to fix this soon.
> 
> Personally I prefer (1) because it looks simpler but I'd be happy with
> (2) as well.
> 
> Ömer
> ___
> ghc-devs mailing list
> ghc-devs@haskell.org
> http://mail.haskell.org/cgi-bin/mailman/listinfo/ghc-devs

___
ghc-devs mailing list
ghc-devs@haskell.org
http://mail.haskell.org/cgi-bin/mailman/listinfo/ghc-devs