Thanks for this! The code looks good to me, and we do need to either do this or partially revert the change in newtype and move the methods back into the class. I think since we are breaking basically every library anyway it’s probably a good opportunity to move the methods out of Newtype class and update the deriving here; generally the main risk with this sort of change is awful errors if you don’t have the right version of the compiler for the library version you’re using, but in this case I think that risk is fairly low.

This also benefited from turning HiddenConstructors warnings for types with Newtype instances into errors, so that #3927 would not suggest to import unexported constructors.

I don’t understand this, can you rephrase?

I’m ambivalent about adding Coercible to the instance context; I feel like you probably shouldn’t be declaring Newtype instances at all if the constructor isn’t part of your public API.

I tried adding a Coercible constraint, but, bizarrely, this doesn't work:

newtype T = T Int
instance newtypeT :: Coercible T Int => Newtype T Int

Yielding an error: No type class instance was found for Prim.Coerce.Coercible T Int

While the following works just fine:

newtype T = T Int

x :: T
x = coerce 42

There must be a different bug somewhere.

But regardless, I think perhaps disallowing hidden constructors in the presence of Newtype instance should accomplish the same thing, no?

But this, on the other hand, does work:

newtype T a = T a
instance newtypeT :: Coercible (T a) a => Newtype (T a) a

@hdgarrood If we add Coercible constraints to the context of derived Newtype instances, and a module declares a newtype with a Newtype instance but hide its constructor:

module Example.N (N) where

import Data.Newtype (class Newtype)
import Prim.Coerce (class Coercible)

newtype N a = N a

instance newtypeN :: Coercible (N a) a => Newtype (N a) a

The compiler will raise a warning the first time it compiles the module, then trying to wrap or unwrap the newtype elsewhere will fail with an error suggesting to import the newtype constructor but this won’t solve anything since the newtype constructor isn’t exported in the first place.

Turning the HiddenConstructors warning into an error for types with a Newtype instance allow us to fail earlier and not suggest things that cannot actually fix the issue.

@fsoikin Could you provide a self contained example exhibiting the failure? The following compiles fine on your branch for me:

module Example where

import Prim.Coerce (class Coercible)
import Data.Newtype (class Newtype)

newtype T = T Int

instance newtypeT :: Coercible T Int => Newtype T Int

But regardless, I think perhaps disallowing hidden constructors in the presence of Newtype instance should accomplish the same thing, no?

@fsoikin It doesn’t: newtypes can be exported with their constructor from internal modules, then re-exported without their constructor from public modules. Adding a Coercible instance to the context of derived Newtype instances ensures that such internal newtypes cannot be freely wrapped and unwrapped outside libraries internals.

Granted those internal newtypes shouldn’t have Newtype instances in the first place, as Harry said, so perhaps we shouldn’t bother.

Oooooh, I'm a moron: I misinterpreted the error message. It was failing on the consumer side, not at instance declaration.

Anyway, I made HiddenConstructors an error for the Newtype class (but left it a warning for Generic).

I do still think that this allows us not to add Coercible as a constraint for Newtype derivation.
Ok, I stand corrected. Will add that in a sec...

But here's another thought: even if we add Coercible to instances created with derive instance ..., we would still be free to use the non-derive declaration, as in instance foo :: Newtype Bar Baz, and it will have the same problem.

But as I think more about it, it kinda seems more like an ergonomic problem. There won't be any sort of bad code generation or anything bad like that as a result.

What do you think?

The compiler will raise a warning the first time it compiles the module, then trying to wrap or unwrap the newtype elsewhere will fail with an error suggesting to import the newtype constructor but this won’t solve anything since the newtype constructor isn’t exported in the first place.

I’m still a bit confused by this discussion. This feels like a bug in the compiler to me, even? If we have

class Coercible a t <= Newtype a t

then shouldn’t it be possible to coerce between two types with a Newtype instance anywhere the Newtype instance is in scope?

I don’t want to make HiddenConstructors an error (yet).

That is: given

newtype N a = N a
instance Newtype (N a) a

then I think it should be possible to coerce N a to a even if only the type is in scope, because the Newtype instance carries a Coercible instance around with it.

@hdgarrood the difference is in where the Coercible instance is required. If you add the constraint, then whoever calls wrap has to be able to produce a Coercible, which means they have to have the constructors. If you don't add the constraint, then the Coercible dictionary is required at the Newtype instance declaration, thus allowing consumers to use it freely without necessarily having the Coercible.

Come to think of it, this kinda looks like the old behavior to me: if you declare a Newtype, then it's the same as exporting the constructor.

I'm personally more or less indifferent on this point, but I lean more towards @hdgarrood's side.

I already made HiddenConstructors an error, but I can easily undo. Just let me know what the decision is.

Also, I think the potential inconsistency between derive instance ... and instance ... declarations is an argument in @hdgarrood's favour.

We should only bother with Coercible constraints in the context of Newtype instances if we want to disallow newtypes whose constructor is only exported for internal use to be freely wrapped and unwrapped by consumers:

module Example.Internal where

import Data.Newtype (class Newtype)

newtype N a = N a
instance newtypeN :: Newtype (N a) a

module Example.Public (module Example.Internal) where

import Example.Internal (N)

module Example where

import Example.Public (N)
import Data.Newtype (wrap)

example :: forall a. a -> N a
example = wrap

If we want to disallow this example then we have to add a Coercible (N a) a constraint to the context of the newtypeN instance, otherwise we don’t have to bother.

This scenario was already allowed before the changes, right?
So it would, in fact, be another breaking change to disallow it now.

Yes, disallowing this would require newtypes constructors to be imported for wrap, unwrap and other newtype combinators to be usable 😬

I’m fine with deriving Newtype instances without Coercible constraints in their context, sorry if was ambiguous about this, I just thought it was a subtle point to consider.

Anyway, since Harry doesn’t want the HiddenConstructors warnings raised for Newtype instances to be turned into errors yet let’s drop 3541d18 and merge this.

Done, dropped.

So it would, in fact, be another breaking change to disallow it now.

Yes, that’s right. I think for me the main reason not to add the contexts is to avoid breaking existing code (or disallowing future code) which makes use of Newtype instances for types whose constructors are public but just happen not to be in scope there.