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

...

...

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

...

...

one to update an environment because the program type checks and does

...

...

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

wrote: the wrong the wrong 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

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

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