Fixing type synonyms to Uniq(D)FM newtypes

classic Classic list List threaded Threaded
9 messages Options
Reply | Threaded
Open this post in threaded view
|

Fixing type synonyms to Uniq(D)FM newtypes

Ömer Sinan Ağacan
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
[hidden email]
http://mail.haskell.org/cgi-bin/mailman/listinfo/ghc-devs
Reply | Threaded
Open this post in threaded view
|

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

Richard Eisenberg-5
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 <[hidden email]> 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
> [hidden email]
> http://mail.haskell.org/cgi-bin/mailman/listinfo/ghc-devs

_______________________________________________
ghc-devs mailing list
[hidden email]
http://mail.haskell.org/cgi-bin/mailman/listinfo/ghc-devs
Reply | Threaded
Open this post in threaded view
|

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 <[hidden email]>, 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 <[hidden email]> 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
> > [hidden email]
> > http://mail.haskell.org/cgi-bin/mailman/listinfo/ghc-devs
>
_______________________________________________
ghc-devs mailing list
[hidden email]
http://mail.haskell.org/cgi-bin/mailman/listinfo/ghc-devs
Reply | Threaded
Open this post in threaded view
|

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 <[hidden email]> 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 <[hidden email]>, 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 <[hidden email]> 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
> > [hidden email]
> > http://mail.haskell.org/cgi-bin/mailman/listinfo/ghc-devs
>
_______________________________________________
ghc-devs mailing list
[hidden email]
http://mail.haskell.org/cgi-bin/mailman/listinfo/ghc-devs

_______________________________________________
ghc-devs mailing list
[hidden email]
http://mail.haskell.org/cgi-bin/mailman/listinfo/ghc-devs
Reply | Threaded
Open this post in threaded view
|

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

Ben Gamari-2
In reply to this post by Ömer Sinan Ağacan
Ömer Sinan Ağacan <[hidden email]> 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
[hidden email]
http://mail.haskell.org/cgi-bin/mailman/listinfo/ghc-devs

signature.asc (497 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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 <[hidden email]> wrote:

>
> Ömer Sinan Ağacan <[hidden email]> 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
> [hidden email]
> http://mail.haskell.org/cgi-bin/mailman/listinfo/ghc-devs
_______________________________________________
ghc-devs mailing list
[hidden email]
http://mail.haskell.org/cgi-bin/mailman/listinfo/ghc-devs
Reply | Threaded
Open this post in threaded view
|

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

Ben Gamari-2
Matthew Pickering <[hidden email]> 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
[hidden email]
http://mail.haskell.org/cgi-bin/mailman/listinfo/ghc-devs

signature.asc (497 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Richard Eisenberg-5
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 <[hidden email]> wrote:
>
> Matthew Pickering <[hidden email]> 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
> [hidden email]
> http://mail.haskell.org/cgi-bin/mailman/listinfo/ghc-devs

_______________________________________________
ghc-devs mailing list
[hidden email]
http://mail.haskell.org/cgi-bin/mailman/listinfo/ghc-devs
Reply | Threaded
Open this post in threaded view
|

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

Ben Gamari-2
Richard Eisenberg <[hidden email]> 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

_______________________________________________
ghc-devs mailing list
[hidden email]
http://mail.haskell.org/cgi-bin/mailman/listinfo/ghc-devs

signature.asc (497 bytes) Download Attachment